Allow LoadRegion to be masked #1261
Conversation
| if ( region.mask ) { | ||
|
|
||
| mask = true; | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure about this logic. It looks like if any region is specified as a "mask" then any region is used as such, which doesn't seem right.
There was a problem hiding this comment.
Just to be sure we have the same understanding for mask, mask=true for a specific region means that only the tiles intersecting that region will be displayed.
I see your concerns now that I am thinking about it. Basically the question is: in case two regions overlap and they have conflicting mask flag, which region should take precedence? Let me think a bit about it and propose a solution.
edit: previous comment content was completely wrong after further thinking, now I understand what you mean.
There was a problem hiding this comment.
Here's my expectation for this feature:
- By default a "region" shape will allow for loading tiles that intersect it up to a target screenspace error - useful for loading tiles in a target region to a certain level of detail or visualization, raycasting, sampling, export, etc.
- If a shape is marked as "mask" then only tiles that will be loaded due to camera frustums or other regions that also intersect a "mask" shape should be displayed.
I don't want to deal with complex boolean operations ordering so I think it's more than sufficient to consider all masked regions to be the same. This means that visibility from non-mask regions should be evaluate and then filtered down based on the mask values (or vice versa).
| raycastTile( tile, scene, raycaster, intersects ) { | ||
|
|
||
| return ! tile.__inFrustum; | ||
|
|
There was a problem hiding this comment.
Why is this needed? This function is intended for overriding raycasting which shouldn't be needed in this plugin.
There was a problem hiding this comment.
I noticed that without overriding, the tiles, even masked, would still be hit by raycasting (during GlobeControls events for instance). The pivot point should not be on hidden tiles, IMO. My particular use case is that two tilesets are layered at the same location (Google 3d tiles and Switzerland-specific dataset). If I want to hide google tiles over Switzerland, I don't want google 3d tiles to be hit by raycasting (it looks like they are higher altitude, probably due to geoid/ellipsoid mismatch).
There was a problem hiding this comment.
The TilesRenderer has a concept of "active tiles" which are tiles that are loaded and represent data that is technically present but not rendered. This allows for raycasting to surfaces offscreen which GlobeControls and EnvironmentControls use, for example, to prevent the the camera from sinking through the tile set surface. Limiting raycasting to only tiles in the frustum will break this behavior.
I understand what you're interested in but I think for now we'll have to leave this up to end users to manage (eg filter hits that are within these mask volumes) and wait to see more about the uses cases these feature is used for. I'll try to think through it a bit more, as well.
There was a problem hiding this comment.
Ok, I reverted. Any chance you can elaborate on how end users (me, in that case) could handle hit filtering, particularly with GlobeControls?
I don't think current public API allows anything like this without a custom GlobeControls, right? As a first step, I tried by using globeControls.raycaster.layers.set(targetTilesetLayerId), but from what I can see GlobeControls.raycaster is private API (at least not exposed in types definitions), right?
| @@ -72,6 +80,14 @@ export class LoadRegionPlugin { | |||
| target.inView = visible; | |||
| target.error = maxError; | |||
|
|
|||
| return ( ( ! visible && mask ) ? false : null ); // Returns false if should force mask the tile. | |||
There was a problem hiding this comment.
Instead of returning a boolean we should assign "inView" on the target object to align with the existing API:
- "true" means the tile is in view and should be traversed.
- "false" means the tile is not in and should not be traversed (even if it's in the camera view).
- "null" means not in view of the regions but do not override the camera traversal.
There was a problem hiding this comment.
Ok, sorry. I was not sure about the API here and whether you wanted it to be updated, based on your previous comments encouraging us to create a PR about it. Getting my head around it and fixing it ASAP.
There was a problem hiding this comment.
Could you please clarify if the three cases you described above are current or future API? Because for me current API doesn't support inView=null anywhere (so I guess this is future API). But, based on my experiments before these changes, a tile with inView=false can still be rendered. So it would mean introducing a breaking change where inView=false no indicates that the tile must not be rendered? Or did I misunderstood your explanation?
In any case, I am currently experiencing difficulties implementing it "your way". A comparison of the API before/after these changes would help. Thanks a lot!
There was a problem hiding this comment.
Because for me current API doesn't support inView=null anywhere (so I guess this is future API)
That's right this isn't used anywhere at the moment. This would mean adding a "null" option rather than a return value from this function.
But, based on my experiments before these changes, a tile with inView=false can still be rendered. So it would mean introducing a breaking change where inView=false no indicates that the tile must not be rendered?
Yeah that's true - admittedly null and false is a bit ambiguous, as well. I suppose ideally we might allow for exporting enums like IN_REGION, OUT_OF_REGION, and MASKED_OUT where the last option means "don't render this tile even if it's in the camera frustum". My explanation above would map to "true", "null", and "false". "null" seems to map to "OUT_OF_REGION" in my mind because it basically means "don't do anything" and just follow the camera frustums.
Regarding breaking changes - some of these plugin callbacks are still changing quite a bit as we find what the useful hooks are for the renderer and they're not documented so I generally feel okay with a breaking change to the return values from the "calculateTileViewError" here. But I'll ping @jsulli and @WilliamLiu-1997 since they're the two I know have been interested in this plugin specifically for some more thoughts.
There was a problem hiding this comment.
Okey, all makes sense now. Thanks a lot for the confirmation.
I have now updated the API as proposed. I just assumed the enum would be an improvement and not yet for this PR. However, I find it very error prone to have a boolean with a meaningful null value. I therefore added a few warnings to make sure bugs are not introduced in the future by forgetting about this. The enum would indeed be cleaner and make everything more explicit. Let me know if you want that in this PR. But first please just check the existing logic :)
| @@ -6,6 +6,7 @@ export class LoadRegionPlugin { | |||
| constructor() { | |||
|
|
|||
| this.name = 'LOAD_REGION_PLUGIN'; | |||
| this.priority = - 1000; | |||
There was a problem hiding this comment.
Why did the priority change?
There was a problem hiding this comment.
See comment above regarding raycasting. BatchedTilesPlugin also overrides raycastTile, so I needed LoadRegionPlugin's raycastTile to take precedence. Actual value was arbitrary and based on other plugins. It just needs to be lower than any other plugin overriding raycastTile.
Following discussion in #1116, I am finally opening a PR with my proposal.
It is tested and working for my own use case (explained in the issue); let me know if it fits in the direction you want for the project :)