-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Per-feature post-processing #6476
Conversation
…stle example for picking.
@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
@mramato Is there a way to get a primitive from an entity? I'd like to support adding an entity or a |
Another suggested TODO, but could be in another PR:
|
I got about half way through reviewing and should finish tomorrow. But ignoring the actual post processing aspect, which I haven't looked at yet, I really love this PR so far. The fact that we no longer have to worry about creating separate draw and pick commands/shaders for everything is really nice! My one question so far is whether there are concerns about performance, like if we had any pick shaders that were optimized to do picking fast. I didn't notice any right away but you might have a better idea. I suppose if we needed the fast path in the future we could have a GLSL define for pick pass vs. draw pass (or something like that). |
There are two possible pick shaders that can be generated. If the fragment shader has any discards, it's the same shader with the color changed to the pick color. Otherwise, a new shader is generated just writes the pick color. |
Not really. Edge detection finds the edge. We could add another pass to expand the edges, but the filter would be twice the length. For example, if you want a width of 5 pixels, that would be a 10x10 filter or 100 texture reads per fragment. |
…re not supported in the Sandcastle example.
…ssing Sandcastle example.
I need to think about this. It doesn't work right now because translucent geometry doesn't write depth. |
@lilleyse Updated. Everything besides translucency, treating a whole tileset as a feature, and edge length has been fixed. The whole tileset as a feature and maybe translucency are doable, but I would open issues for them. |
@lilleyse I re-targeted this to merge in master since the post-processing branch was merged. |
Ok, yeah open issues for those two. Edge length doesn't sound worth it. The recent fixes work well. Given the large architectural changes let's merge after the June release. |
Just did a quick check to make sure everything was still functioning. Thanks @bagnell. |
Late to the party, but still going to add a few comments here. |
Are the dots expected on my Macbook/Intel? http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles%20Feature%20Picking.html |
Is this Sandcastle example - and silhouettes in general - supposed to work in 2D? It's OK if it isn't, but let's document it if not already done. |
} | ||
}); | ||
|
||
var collection = viewer.scene.postProcessStages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collection
?
Be precise.
What should be the canonical name used throughout? stages
, right?
var blackAndWhite = collection.add(Cesium.PostProcessStageLibrary.createBlackAndWhiteStage()); | ||
blackAndWhite.uniforms.gradations = 5.0; | ||
|
||
if (!silhouette.isSupported(viewer.scene)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it odd that a user needs to create an instance to check isSupported
? Is there clean and obvious way to avoid this with static isSupported
somewhere appropriate like GroundPrimitive.isSupported
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How else would it work? There isn't a class for each of the stages. If there was, we could do something like Silhouette.isSupported
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
if (!Cesium.PostProcessStageLibrary.isSilhouetteSupported(viewer.scene)) {
Slightly more coupled, but cleaner for the end user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, db5cb75
handler.setInputAction(function(movement) { | ||
var pickedObject = viewer.scene.pick(movement.endPosition); | ||
if (Cesium.defined(pickedObject)) { | ||
stage.selectedFeatures = [pickedObject.primitive]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure selectedFeatures
is the right term? What all can this be? A primitive, entity, 3D tileset feature?
Maybe just selected
or something like appliesTo
?
### 1.47 - 2018-07-02 | ||
|
||
##### Additions :tada: | ||
* `PostProcessStage` has a `selectedFeatures` property which is an array of primitives used for selectively applying a post-process stage. In the fragment shader, use the function `bool czm_selected(vec2 textureCoordinates` to determine whether or not the stage should be applied at that fragment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the fragment shader, use the function
bool czm_selected(vec2 textureCoordinates
to determine whether or not the stage should be applied at that fragment.
This is too much detail for CHANGES.md. Just have this documented where needed. Maybe a Sandcastle example with a custom stage as well.
|
||
##### Additions :tada: | ||
* `PostProcessStage` has a `selectedFeatures` property which is an array of primitives used for selectively applying a post-process stage. In the fragment shader, use the function `bool czm_selected(vec2 textureCoordinates` to determine whether or not the stage should be applied at that fragment. | ||
* The black-and-white and silhouette stages have per-feature support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide the specific name of the stage in CesiumJS.
Also, what is the plan for per-feature bloom - isn't that where we want to be? What is stopping us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think per-feature bloom makes sense. With HDR, you could change the luminance so certain geometry has bloom while other don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user wants to implement mouse-over glow for an object that bleeds into the scene on the edges, how do they do it? We expose luminance as part of the material and they just change it? Does it need a blur post-process?
return; | ||
} | ||
|
||
if (scene._logDepthBuffer && defined(command.derivedCommands.logDepth)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but why not var derivedCommands = command.derivedCommands
to remove a lot of redundancy below?
The smearing is expected.
The dots are not. |
If this is specific to Mac/Intel, probably OK to just submit an issue. |
I see some dots as well. Seems to be from some of the small seams in the geometry. I wonder... could the silhouette shader be changed so that it outlines only the edge of the feature and not the interior sections? If the post process shader has access to the id texture it could ignore checking depths for pixels that share the same id as the current pixel. This would fix both the dots and the smearing. |
@bagnell curious if you have any thoughts on this approach. |
NOTE this is targeting the
post-processing-1.0
branch.Adds per-feature post-processing. Each post-process stage has a
selectedFeature
property that is an array of primitives (Model
,Primitive
,Billboard
, etc.) orCesium3DTileFeature
s. The only two stages with built-in support are the silhouette and black and white stages. Any classification primitives are not supported.See the
Per-Feature Post Processing
and3D Tiles Feature Picking
Sandcastle examples.TODO:
Add support for entities.