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

Fixed label background rendering. #11293

Merged
merged 8 commits into from
May 31, 2023
Merged

Fixed label background rendering. #11293

merged 8 commits into from
May 31, 2023

Conversation

IKangXu
Copy link
Contributor

@IKangXu IKangXu commented May 18, 2023

If the initial label background color is set to false and then dynamically modified to true, rendering problems will occur.

Before repair
image

After repair
image

@cesium-concierge
Copy link

Thank you so much for the pull request @IKangXu! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ggetz
Copy link
Contributor

ggetz commented May 18, 2023

Hi @IKangXu.

In order to review this PR, we will need the following:

Thanks!

@IKangXu
Copy link
Contributor Author

IKangXu commented May 19, 2023

Hi @IKangXu.

In order to review this PR, we will need the following:

Thanks!

✅ CONTRIBUTORS.md entry was updated
✅ CLA was confirmed
✅ CHANGES.md was updated

@IKangXu
Copy link
Contributor Author

IKangXu commented May 19, 2023

example

@IKangXu
Copy link
Contributor Author

IKangXu commented May 22, 2023

@ggetz

@ggetz
Copy link
Contributor

ggetz commented May 22, 2023

Thanks @IKangXu! We will review shortly, with the intention of getting this into the next release.

@IKangXu
Copy link
Contributor Author

IKangXu commented May 22, 2023

@ggetz Thanks a lot.

@ggetz
Copy link
Contributor

ggetz commented May 26, 2023

Hi @IKangXu, I can confirm this fixes the issue. However, I think there may be a fix that is a bit simpler.

I believe we can get the same effect with this code change. This would remove the need for the additional getSubRegionIndex function. What do you think?

Also, please resolve the conflicts in CHANGES.md when you can.

Thanks again!

@IKangXu
Copy link
Contributor Author

IKangXu commented May 27, 2023

Hi @ggetz, If getSubRegionIndex is deleted, it will require frequent waiting for imageIndexPromise, Actually, these have already been added to the atlas

As the annotation in the figure states.
image

CHANGES.md file conflict resolved.

@ggetz
Copy link
Contributor

ggetz commented May 30, 2023

@IKangXu In the case where an image is already created, .addImage will return early with an existing promise if available rather than creating any new resources. This will take a frame to resolve, but ensures that any in-process additions do finish before continuing.

Can we do the same early return for addSubRegion instead of creating a whole new function? It keeps the API footprint smaller and simplifies the control flow in _loadImage.

@IKangXu
Copy link
Contributor Author

IKangXu commented May 31, 2023

@IKangXu In the case where an image is already created, .addImage will return early with an existing promise if available rather than creating any new resources. This will take a frame to resolve, but ensures that any in-process additions do finish before continuing.

Can we do the same early return for addSubRegion instead of creating a whole new function? It keeps the API footprint smaller and simplifies the control flow in _loadImage.

@ggetz I agree with your proposal and it has been fixed.

@ggetz
Copy link
Contributor

ggetz commented May 31, 2023

Thanks @IKangXu!

@ggetz ggetz merged commit b741605 into CesiumGS:main May 31, 2023
@brunokunace
Copy link

@ggetz do you have any idea when a new version with this fix will be created?

@ggetz
Copy link
Contributor

ggetz commented Jun 12, 2023

@brunokunace The fix went out in version 1.106 on June 1.

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.

4 participants