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

Make sampleTerrain behaviour consistent with the rejectOnTileFail flag #11998

Merged
merged 3 commits into from
May 30, 2024

Conversation

jesseyay
Copy link
Contributor

@jesseyay jesseyay commented May 22, 2024

Description

Passes through the rejectOnTileFail flag so it will be respected if any tile requests fail, not just if the first request fails. Note that the linked issue discusses changing the default of this flag to true which I tried, but it ends up failing the test fills in what it can when given a mix of positions with and without valid tiles

Issue number and link

Addresses/Related to: #11694

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jesseyay!

✅ We can confirm we have a CLA on file for you.

@jesseyay jesseyay marked this pull request as ready for review May 22, 2024 04:32
@chris-cooper
Copy link
Contributor

@ggetz Would it be possible for you or someone on the team to have a look at this as we notice some tile errors are still slipping through unnoticed.

@ggetz
Copy link
Contributor

ggetz commented May 28, 2024

Thanks @jesseyay! We'll review shortly with the intention of including this in the next CesiumJS release.

@ggetz
Copy link
Contributor

ggetz commented May 29, 2024

Thanks @jesseyay! The code changes here look great.

Do you or @chris-cooper have any thoughts about reversing the default of rejectOnTileFail to true given #11694? That would restore behavior that broke in 1.92.

@chris-cooper
Copy link
Contributor

Do you or @chris-cooper have any thoughts about reversing the default of rejectOnTileFail to true given #11694? That would restore behavior that broke in 1.92.

That makes sense if it was the old behaviour. Do you want to remove the flag as part of this PR @jesseyay ?

@jesseyay
Copy link
Contributor Author

Hey @ggetz thanks for taking a look!

I tried changing the default in this PR for rejectOnTileFail to true but there is this test which looks like it is 10 years old that will fail if we change it 🤔

I did a bit of further digging and I suppose the behaviour with a 404 response depends both on the rejectOnTileFail flag and on the terrainProvider that gets passed in, so maybe the regression #11694 originates in a terrain provider?

@ggetz
Copy link
Contributor

ggetz commented May 30, 2024

Got it, thanks for looking into that @jesseyay! Sounds like we'll need to look a bit closer to find the source of the regression. No need to hold up this PR for that.

@ggetz ggetz merged commit 4f41c01 into CesiumGS:main May 30, 2024
4 checks passed
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

3 participants