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

3D Tiles - Refinement based on proximity #4138

Merged
merged 3 commits into from Sep 30, 2016
Merged

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jul 19, 2016

For CesiumGS/3d-tiles#101

Not ready until #4130 is ready.

If viewerRequestVolume has multiple sub-properties, the viewer needs to be in the union of them?

I'm not handling this right now. We don't handle it for our other bounding volumes either so maybe we should wait to do them all at the same time.

For replacement refine, I think it is fine to refine the parent and only render the children that base the new volume test (even if it isn't all of them), but we should refine the parent until at least one child is ready to render and visible.

For the sake of making the implementation super easy, I left out this point for now.

To do:

In action, and working with the transform property:
capture
capture2

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

If viewerRequestVolume has multiple sub-properties, the viewer needs to be in the union of them?

I'm not handling this right now. We don't handle it for our other bounding volumes either so maybe we should wait to do them all at the same time.

Should be trivial, just check each with ||, but....maybe we should just change the spec to allow only one sub-property (for this and all BVs. We could loosen it later if there is a good use case.

What do you think?

We don't handle it for other bounding volumes because for them the spec says that it is OK to use any bounding volume.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

Hopefully this was obvious, but this

For replacement refine, I think it is fine to refine the parent and only render the children that base the new volume test (even if it isn't all of them), but we should refine the parent until at least one child is ready to render and visible.

should have been:

For replacement refine, I think it is fine to refine the parent and only render the children that pass the new volume test (even if it isn't all of them), but we shouldn't refine the parent until at least one child is ready to render and visible.

OK to wait on this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

Added above:

  • Add debug volume button to Sandcastle example.
  • Unit tests

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

Also added:

  • Example dataset for Sandcastle example.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

Looks good!

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 24, 2016

If viewerRequestVolume has multiple sub-properties, the viewer needs to be in the union of them?

I'm not handling this right now. We don't handle it for our other bounding volumes either so maybe we should wait to do them all at the same time.

Should be trivial, just check each with ||, but....maybe we should just change the spec to allow only one sub-property (for this and all BVs. We could loosen it later if there is a good use case.

I changed the spec so that only on property will be in any boundingVolume object.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 24, 2016

For replacement refine, I think it is fine to refine the parent and only render the children that pass the new volume test (even if it isn't all of them), but we should refine the parent until at least one child is ready to render and visible.

For the sake of making the implementation super easy, I left out this point for now.

Looking at this again, the request volume test only applies to the parent so if it passes, replacement refinement continues as usual with no special cases needed. Implicit in this is some or all of the children may have request volume tests that fail. See how these feels in practice and we'll update the spec accordingly.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 9, 2016

@lilleyse should this be retargeted to 3d-tiles so we can delete the 3d-tiles-transform branch?

@lilleyse lilleyse changed the base branch from 3d-tiles-transform to 3d-tiles September 9, 2016 13:41
@lilleyse
Copy link
Contributor Author

This is now ready.

I added 1513fbb to address #4138 (comment), but I'm not sure if the overhead is worth it.

@lilleyse
Copy link
Contributor Author

@pjcozzi Ready when you have a chance.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 30, 2016

I added 1513fbb to address #4138 (comment), but I'm not sure if the overhead is worth it.

Looks OK to me. We'll reconsidered later if need be.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 30, 2016

Part of #3241

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 30, 2016

Looks good!

@pjcozzi pjcozzi merged commit 9be9885 into 3d-tiles Sep 30, 2016
@pjcozzi pjcozzi deleted the 3d-tiles-proximity branch September 30, 2016 01:45
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.

None yet

2 participants