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

Tiles traverser: do not check for visibility in canTraverse. #3032

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

Avnerus
Copy link
Collaborator

@Avnerus Avnerus commented Jun 24, 2024

Hi!
Yet another fix for tile traversal. Bug was noticed while traveling in Google's 3D Tiles.
The canTraverse function should not check for visibility in the traversal process, as shown in the CesiumJS code. Probably there are cases where a parent in the traversal process is not visible, but the child tile is.
Here is how it looked before the fix:
canTraverse-bug
And here is how it looks after:
canTraverse-fix
Possibly, this functionality existed previously via the ignoreVisibility argument. Now I removed both optional arguments from canTraverse since they are not used.

If approved, would it be possible to merge this PR to the current stable branch?

Thank you,
/Avner

@Avnerus Avnerus requested a review from ibgreen June 24, 2024 07:13
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I see that there was a flag already. From a quick look I don't see that flag documented or referenced elsewhere, so should be fine to just remove it.

@Avnerus
Copy link
Collaborator Author

Avnerus commented Jun 26, 2024

Thank you! Do you think we can merge this to the stable branch?
@felixpalmer @belom88

@ibgreen
Copy link
Collaborator

ibgreen commented Jun 26, 2024

Thank you! Do you think we can merge this to the stable branch? @felixpalmer @belom88

To prepare for a possible appropval, you could make a cherry-pick PR against 4.2-release branch.

@ibgreen ibgreen merged commit 6b41594 into master Jun 26, 2024
1 check passed
@ibgreen ibgreen deleted the fix-tiles-can-traverse branch June 26, 2024 13:49
@Avnerus
Copy link
Collaborator Author

Avnerus commented Jun 27, 2024

Thank you.
I opened the cherry-picked PR at #3039.
It seems to be failing the test but with a reason not related to the commit.

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