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
3D Tiles - refine to visible children in replacement refinement #4287
Conversation
Can you please test this with several different datasets to get a feel for how bad it can be so we know how to prioritize clipping parents with their loaded children? |
Add a comment for this inside |
More like 14%, right? Also, what is the Total column? It is always 341. |
@@ -294,6 +294,13 @@ define([ | |||
this.replaced = false; | |||
|
|||
/** | |||
* The stored plane mask from the visibility check during tree traversal. | |||
* | |||
* @type {Boolean} |
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 this CullingVolume
?
@@ -536,6 +514,42 @@ defineSuite([ | |||
}); | |||
}); | |||
|
|||
it('replacement refinement - refines to visible ready children', function() { |
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.
With all the tileset traversal changes, are you sure this is the only new test we need? Are all cases covered?
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.
Even though the test seems simple it does cover all the new traversal cases.
Hah yeah, I updated the comment.
Changed the name from "Total" to "Total tiles in tileset". It's always 341. |
Some datasets are more noticeable than others, I think we will need to explore parent clipping to make this better. |
Updated. I still need to add a flag to enable/disable this optimization. |
Will review and merge when this is ready. |
Updated. |
@@ -536,6 +514,42 @@ defineSuite([ | |||
}); | |||
}); | |||
|
|||
it('replacement refinement - refines to visible ready children', function() { | |||
viewRootOnly(); | |||
return Cesium3DTilesTester.loadTileset(scene, tilesetUrl).then(function(tileset) { |
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.
Run this test twice, once with refineToVisible
set to true
and once with it set to false
?
Do you get these test failures with http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/3d-tiles-refine-visible/Specs/SpecRunner.html?spec=Scene%2FCesium3DTileset
|
Just those comments. |
Those test failures are fixed now, and a new test is added. |
Thanks @lilleyse! Huge improvement here. |
@austinEng can you verify if this is related to the child-union optimization? |
@JPhPons if you pull the latest 3d-tiles branch and construct the tileset with |
Hi Sean,
Yes, I still have the same problem after proceeding as you suggested.
JP
From: Sean Lilley [mailto:notifications@github.com]
Sent: mercredi 1 mars 2017 21:12
To: AnalyticalGraphicsInc/cesium <cesium@noreply.github.com>
Cc: Jean-Philippe Pons <Jean-Philippe.Pons@bentley.com>; Mention <mention@noreply.github.com>
Subject: Re: [AnalyticalGraphicsInc/cesium] 3D Tiles - refine to visible children in replacement refinement (#4287)
@JPhPons<https://github.com/JPhPons> if you pull the latest 3d-tiles branch and construct the tileset with cullWithChildrenBounds as false, do you see the same artifacts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#4287 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AY6Am9eXG_jZ6jnoDteugNDSwJjU7WRvks5rhdD-gaJpZM4J2Snf>.
|
Ok thanks for checking. Did you start to experience this problem recently? Is it occurring with other tilesets? It reminds me of issues we used to see with replacement refinement with empty tiles (like #3517). Feel free to send me the tileset for testing. |
Hi,
Sorry for the late answer, I finally had time to look into it!
I seems to be a side-effect of the pull request #3517, which solved only partially our problems with external tilesets. The problem has been there since then, it is a shame I didn’t notice it in my tests earlier…
Please find attached a patch which fixes the problem. I am not confident enough to submit it before getting @lilleyse<https://github.com/lilleyse>‘s feedback. The patch can be tested in this demo:
https://d3h9zulrmcj1j6.cloudfront.net/CesiumMeasurements/index.html
JP
De : Patrick Cozzi [mailto:notifications@github.com]
Envoyé : lundi 6 mars 2017 17:26
À : AnalyticalGraphicsInc/cesium <cesium@noreply.github.com>
Cc : Jean-Philippe Pons <Jean-Philippe.Pons@bentley.com>; Mention <mention@noreply.github.com>
Objet : Re: [AnalyticalGraphicsInc/cesium] 3D Tiles - refine to visible children in replacement refinement (#4287)
@JPhPons<https://github.com/JPhPons> @lilleyse<https://github.com/lilleyse> any update here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#4287 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AY6AmyRInEZXQ5osFOQAPW34ROcIgYF6ks5rjDObgaJpZM4J2Snf>.
|
The attachment doesn't seem to go through with github replies, but you can send it to me directly or otherwise open a PR. As a side note, the 3d-tiles traversal is about to get a large overhaul and likely fixes the issues here - #5128 |
Thanks for sending it over. The diff seems fine as an interim fix. Would you like me to open a PR? |
I am still a newbie with github so that would be very helpful! Thanks,
JP
De : Sean Lilley [mailto:notifications@github.com]
Envoyé : vendredi 31 mars 2017 17:45
À : AnalyticalGraphicsInc/cesium <cesium@noreply.github.com>
Cc : Jean-Philippe Pons <Jean-Philippe.Pons@bentley.com>; Mention <mention@noreply.github.com>
Objet : Re: [AnalyticalGraphicsInc/cesium] 3D Tiles - refine to visible children in replacement refinement (#4287)
Thanks for sending it over. The diff seems fine as an interim fix. Would you like me to open a PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#4287 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AY6Am0xqqQdrMcVvYfDbOkZScDxp_fe6ks5rrR-OgaJpZM4J2Snf>.
|
Opened #5171 |
For #3241
When not all visible children are loaded, I go with a third approach - render the parent and available children together, which is slightly noticeable in some cases but doesn't hurt visual quality since the other visible children usually load in soon after.
Since the visibility check must now happen in the replacement refinement path, I save the result and don't recheck the visibility once the tile is popped from the stack. I modified the additive refinement path to do the same, store the
planeMask
now and use later.I also ended up reverting #4280 because otherwise the bounding volume check will happen twice, first in the
selectTiles
loop, and again inselectTile
. This only applies to tiles without content bounding volumes, which is many replacement tilesets. As long ascontentsVisibility
is called aftervisibility
is already checked, this should be ok.Some numbers:
Now that we don't need to request all children, and instead just the visible ones, there are some memory improvements. In real world tilesets I've noticed between 14% to 50% savings in the number of loaded tiles. These numbers below are for a generic replacement refinement tileset:
Before
After