Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model coloring #4547

Merged
merged 16 commits into from
Dec 6, 2016
Merged

Model coloring #4547

merged 16 commits into from
Dec 6, 2016

Conversation

lilleyse
Copy link
Contributor

I was inspired by this post on the forum to be able to color a model with a solid color.

The blend color is mixed with the rendered colored. It's not possible to just replace the diffuse color unless some semantic is defined, but this is probably not worth the trouble.

Is this something we want to support in the Entity API as well?

truck

airplane

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 30, 2016

Somewhat related to #2387 and #4314.

*
* @default Color.RED
*/
this.blendColor = defaultValue(options.blendColor, Color.RED);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention to allow RGBA values? If so, the render state may need to change on the fly when alpha changes from one to non-one. Likewise, you may want to optimize the case when alpha is zero and not issue the DrawCommand.

If the intention is only RGB, the explicitly document that and throw when alpha is non-zero.

I would support alpha so users can fade in/out a model with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had intentionally left alpha out, but I do think it will be nice to support in this PR.

function modifyShaderForBlendColor(shader) {
shader = ShaderSource.replaceMain(shader, 'czm_blend_main');
shader +=
'uniform vec4 czm_blendColor; \n' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of czm_, should we use something like gltf_ (like we use tile_ in 3D Tiles)? I probably used czm_ elsewhere for 3D models, but perhaps we should reserve it for built-ins.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 30, 2016

Add this to the Sandcastle example.

And yes, I expect we would want Entity API support and eventually CZML.

@pjcozzi pjcozzi mentioned this pull request Nov 28, 2016
6 tasks
@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

Ready for review.

Added Entity API and CZML support. Also added alpha support.

alpha

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

There are still some unavoidable artifacts when OIT is off due to lack of triangle sorting. The back/front commands help though, but can't prevent multiple front faces from being drawn in arbitrary order.

This is with alpha of 0.9 to highlight the issue.
artifacts

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

Another note: glTF that are marked as premultiplied are now un-premultiplied. Besides making it easier to blend colors in this PR, it will also improve OIT which overrides the glTF's render state and always does un-premultiplied alpha blending.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

The back/front commands help though, but can't prevent multiple front faces from being drawn in arbitrary order.

Ah yes, this will only work for convex objects. Perhaps it is not worth it since this is a fallback and only a partial solution that is unlikely to be enough in common cases.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

Ah yes, this will only work for convex objects. Perhaps it is not worth it since this is a fallback and only a partial solution that is unlikely to be enough in common cases.

Ok, then it may be worth changing 3d-tiles as well which also generates translucent front/back commands.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Is Blend Alpha supposed to work when Blend Amount is zero? Blend Color does not.

ok

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

In the Sandcastle example, Blend Color going from 0 to 1 is awkward. If a color picker is trivial, let's use it; otherwise, anything else like a dropdown would be fine.

ok

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Ok, then it may be worth changing 3d-tiles as well which also generates translucent front/back commands.

OK with me, probably my doing.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

Is Blend Alpha supposed to work when Blend Amount is zero? Blend Color does not.

Yes this could be documented better, but it is not tied to Blend Amount because that will change the color in the process. Someone may want to only change the alpha portion.

maximumScale : 20000
maximumScale : 20000,
blendAmount : viewModel.blendAmount,
blendColor : Cesium.Color.fromHsl(viewModel.blendColor, 1.0, 0.5, viewModel.blendAlpha)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do our users think of this as blending? Or just as applying a color to a model like Billboard.color?

What do you think of renaming this to color? I'm not sure what to rename blendAmount to...is there anywhere else in the Cesium API to mirror?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Yes this could be documented better, but it is not tied to Blend Amount because that will change the color in the process. Someone may want to only change the alpha portion.

Couldn't they just set the color to white?

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

Couldn't they just set the color to white?

Not currently because the the model coloring is doing a mix, not a multiply.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Couldn't they just set the color to white?
Not currently because the the model coloring is doing a mix, not a multiply.

I don't want to make this too complicated, but should we follow the same pattern as 3D Tiles features? Do you have use cases for mix? There will be lots of use cases for multiply, e.g., blue/red force.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 5, 2016

I don't want to make this too complicated, but should we follow the same pattern as 3D Tiles features? Do you have use cases for mix? There will be lots of use cases for multiply, e.g., blue/red force.

Yeah I think supporting both modes may be a good idea, similar to Cesium3DTileColorBlendMode. Mix is nice for models that aren't close to white. Like a green model with a red blendColor will be black with multiply.

return translucentCommand;
}

function updateBlendColor(model, frameState) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider doing this as part of Cesium's derived commands? Or perhaps wait until #4314?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes both this and the 2D commands can be derived commands. I'll wait for #4314.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Looks good, just those comments.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 6, 2016

Ready now. I borrowed ColorBlendMode from 3d-tiles so HIGHLIGHT, REPLACE, and MIX are supported.

@@ -19,10 +19,45 @@
<body class="sandcastle-loading" data-sandcastle-bucket="bucket-requirejs.html">
<style>
@import url(../templates/bucket.css);
#toolbar {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lilleyse up to you, but do you think that we've made this Sandcastle example too complicated? Would it be better (and more discoverable) to have separate 3D Models and 3D Model Coloring examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus silhouettes can be combined in that new demo.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 6, 2016

Is the separate Firefox issue, #4658, right?

ok

@@ -34,6 +34,7 @@ define([
'../Core/Spherical',
'../Core/TimeInterval',
'../Core/TimeIntervalCollection',
'../Scene/ColorBlendMode',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the CZML doc or submit an issue to update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

};
}

function createColorBlendFunction(model) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 3D Tiles also use this? If so, should we make this a utility function on ColorBlendMode that takes a blendAmount?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 6, 2016

That's all my comments. This is a masterpiece.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 6, 2016

Ready

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 6, 2016

Just have one small case to add when the alpha is 0.

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 6, 2016

Ready now.

Is the separate Firefox issue, #4658, right?

Yeah

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 6, 2016

CZML doc PR here: AnalyticalGraphicsInc/czml-writer#122

@pjcozzi pjcozzi merged commit 30113c0 into master Dec 6, 2016
@pjcozzi pjcozzi deleted the model-color branch December 6, 2016 19:09
@lilleyse lilleyse mentioned this pull request Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants