Fix for model coloring

[szsolt]

#1 Opened issue

[szsolt]

For some reason the CI system fails to run:

npm run makeZipFile

Running locally the command succeed without any issue.

[lilleyse]

Thanks for catching this @szsolt. This is a good initial solution, with the downside being if there is no result parameter then a new Color will be allocated every time, which actually seems to be a problem with silhouetteColor too currently...

I think the proper solution may be to turn Model.color and Model.silhouetteColor into a getter/setter and mark that the colors as dirty in a different way. I am happy to add that work to this PR, unless you wish to look into it.

[szsolt]

@lilleyse What if we follow how modelMatrix is set a few lines above?

model.silhouetteColor = Property.getValueOrClonedDefault(modelGraphics.silhouetteColor, time, defaultSilhouetteColor, model.silhouetteColor);
model.color = Property.getValueOrClonedDefault(modelGraphics._color, time, defaultColor, model.color);

[szsolt]

Updated pull request to avoid creating a new Color each update.

Is this good enough or you still prefer the getter/setter option?

[pjcozzi]

@szsolt can you please send in a Contributor License Agreement (CLA) so we'll be able to merge this?

[lilleyse]

I think the getter/setter is still preferable. If the model's color is not set the default color will be cloned every update.

[pjcozzi]

@szsolt do you have any update here? We would like to get this fix into master since other users have also noticed it. Do you prefer that @lilleyse or I finish this or are you able to? Thanks!

[szsolt]

Updated the pull request. Now if the model color is not set, it will not be cloned for every update.
Also removed the unnecessary clone from updateSilhouette() as it was cloning the full Color() object and all it needed was the alpha value of it.

I was wondering if it would worth changing the behavior of getValueOrDefault() or introducing a new method which will allocate a new object when the result is frozen, something like:

Property.getValueOrDefaultSafe = function(property, time, valueDefault, result) {
    return defined(property) ? defaultValue(property.getValue(time, Object.isFrozen(result) ? undefined : result), valueDefault) : valueDefault;
};

Of course we could implement isFrozen() method like you did for the freeze() method for older browser compatibility.

[szsolt]

Please feel free to modify / replace my implementation as you guys have more expertise on the implementation detail.

[lilleyse]

Cool nice idea with only storing the alphas to avoid the clone, and using the underscored colors as scratch variables. I think this is good to go for now, if we decide to use getters/setters for Color later that can be in a different PR.

@pjcozzi can you do the final review?

[pjcozzi]

Looks great, thanks again @szsolt! Great job with this.

If you have the time to more involved with Cesium, there's lots of ideas here.

[pikvic]

This bug still appears in 1.96. Link to Sandcastle

[j9liu]

Hi @pikvic,

This bug has already been fixed in main, and the fix will be included in the CesiumJS 1.97 release.