Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 fixes and perf improvements #2143
3D Tiles fixes and perf improvements #2143
Changes from all commits
f888d36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think it's a poor pattern to store in a single variable different type of object (
volume
) then to use another variable (hereinitialVolumeType
) to know how to manipulatevolume
.. (btw you could use instanceof instead of initialVolumeType)you could create a wrapper abstract class called
Volume
implementing a methodisVisible(camera, tileMatrixWorld)
then specifyingSphereVolume
andBoxVolume
(class extendingVolume
) the specific behavior.you could also put two attributes in
C3DTBoundingVolume
instead ofvolume
one calledbox
and the other onesphere
which is the solution I rather, since aC3DTBoundingVolume
could have the both and it avoids to create an abstract classThere 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.
The last pattern was the previously used pattern. However, if we choose this one I would keep the
initialVolumeType
to be able to know if the sphere comes from a bounding sphere or from a bounding region, in case it may be useful for itowns' users in some way. The second pattern is also interesting, I actually thought about it when implementing this. I don't have time to implement it right now but I'll come back to it soon.