Post process updates #6680
Post process updates #6680
Conversation
|
@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. 🌍 🌎 🌏 |
| primitive = primitives.get(i); | ||
| if (primitive.id === entity) { | ||
| remove(); | ||
| break; |
There was a problem hiding this comment.
I guess we don't have a better way to get the primitive from an entity, but this function almost seems too complicated to be part of a Sandcastle demo. Maybe the demo should do picking like the other per-feature post processing demo?
There was a problem hiding this comment.
I thought so too, but I couldn't find any examples where we get the model from an entity. I know this was asked for a few times on the forum so now we can point to this example.
There was a problem hiding this comment.
My main concern is once it's in Sandcastle it's considered the right way to do something. Do we want to endorse this as the way to get the primitive from the entity?
There was a problem hiding this comment.
I mean, I think we on-purpose made it impossible for someone to get the primitive through the Entity API. We generally don't want end-users to interact with the primitive if they're using the Entity API because there can be unexpected side-effects. Maybe we should find a way to enable this via Entity API directly before adding this kind of workaround in a Sandcastle example. @mramato do you have any input here?
There was a problem hiding this comment.
I talked offline with @mramato when I opened the per-feature post process PR. He said we might be able to add a one-to-one mapping for entity to primitive in the future. In the meantime, this was the way to do it. Until we decide what to do I changed this to use picking like the other Sandcastle example.
There was a problem hiding this comment.
I would suggest having scene.pick return the entity and object describing the primitive (ie the Entity and PolygonGraphics) but we can figure that out later. This is fine for now.
There was a problem hiding this comment.
Yeah, there are a lot of dragons here because (by design) the entity data and entity visualization are decoupled and there isn't a one-to-one mapping anywhere in the system. A primitive knows what entity it is representing in a given frame, but an entity can be represented by any number of primitives and it has no idea what those primitives are. There's a lot of good reasons for doing it this way, but there are obvious use cases where we say "I want the Model for this Entity". We can certainly chose to more tightly couple entities and primitives, it would just most likely involve removing flexibility of the Entity API and also probably some breaking changes.
The possibly more correct (yet difficult) thing to do would be to better expose post processing features through the Entity API.
There was a problem hiding this comment.
For example, what about being able to associate a post-process stage to a particular ModelGraphics and everything "just works" under the hood? I haven't dug into the post process architecture yet, so maybe that idea makes no sense.
| * @see {Context#depthTexture} | ||
| * @see {@link http://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/|WEBGL_depth_texture} | ||
| */ | ||
| PostProcessStageComposite.prototype.isSupported = function(scene) { |
There was a problem hiding this comment.
Should PostProcessStage.prototype.isSupported also be removed?
|
@lilleyse This is ready. |
|
Otherwise looks good. |
From comments in #6476.