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

Per-feature post-processing #6476

Merged
merged 50 commits into from
Jun 4, 2018
Merged

Per-feature post-processing #6476

merged 50 commits into from
Jun 4, 2018

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Apr 20, 2018

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.) or Cesium3DTileFeatures. 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 and 3D Tiles Feature Picking Sandcastle examples.

TODO:

  • Add support for entities.
  • Fix tests.
  • Add tests.
  • Update CHANGES.md

@cesium-concierge
Copy link

Signed CLA is on file.

@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@bagnell
Copy link
Contributor Author

bagnell commented Apr 20, 2018

@mramato Is there a way to get a primitive from an entity? I'd like to support adding an entity or a *Graphics object to the selectedFeatures array.

@lilleyse
Copy link
Contributor

Another suggested TODO, but could be in another PR:

  • Remove silhouetting from Model.js because an edge post process can be used instead.

@lilleyse
Copy link
Contributor

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).

@bagnell
Copy link
Contributor Author

bagnell commented Apr 27, 2018

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.

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.

@bagnell bagnell mentioned this pull request May 2, 2018
12 tasks
@bagnell
Copy link
Contributor Author

bagnell commented May 11, 2018

Is it easy to add a configurable silhouette edge length?

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.

@bagnell
Copy link
Contributor Author

bagnell commented May 11, 2018

Would it be possible to add silhouette support to translucent objects?

I need to think about this. It doesn't work right now because translucent geometry doesn't write depth.

@bagnell
Copy link
Contributor Author

bagnell commented May 11, 2018

@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.

@bagnell bagnell changed the base branch from post-processing-1.0 to master May 15, 2018 19:04
@bagnell
Copy link
Contributor Author

bagnell commented May 15, 2018

@lilleyse I re-targeted this to merge in master since the post-processing branch was merged.

@lilleyse
Copy link
Contributor

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.

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.

@bagnell
Copy link
Contributor Author

bagnell commented Jun 4, 2018

@lilleyse This is ready. After this is merged, I'll open the issues.

EDIT: Opened #6656 and #6657.

@lilleyse
Copy link
Contributor

lilleyse commented Jun 4, 2018

Just did a quick check to make sure everything was still functioning. Thanks @bagnell.

@lilleyse lilleyse merged commit 65a382c into master Jun 4, 2018
@lilleyse lilleyse deleted the per-feature-post-process branch June 4, 2018 21:40
@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2018

Late to the party, but still going to add a few comments here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2018

Are the dots expected on my Macbook/Intel?

image

http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles%20Feature%20Picking.html

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2018

Is the smearing here expected?

ok2

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2018

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;
Copy link
Contributor

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)) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bagnell do you plan on adding this in #6680?

Copy link
Contributor Author

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];
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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?

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 don't think per-feature bloom makes sense. With HDR, you could change the luminance so certain geometry has bloom while other don't.

Copy link
Contributor

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)) {
Copy link
Contributor

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?

@bagnell
Copy link
Contributor Author

bagnell commented Jun 12, 2018

Is the smearing here expected?

The smearing is expected.

Are the dots expected on my Macbook/Intel?

The dots are not.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 12, 2018

Are the dots expected on my Macbook/Intel?

The dots are not.

If this is specific to Mac/Intel, probably OK to just submit an issue.

@bagnell bagnell mentioned this pull request Jun 12, 2018
@lilleyse
Copy link
Contributor

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.

dots

@lilleyse
Copy link
Contributor

@bagnell curious if you have any thoughts on this approach.

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.

5 participants