Fix loading of label background

[UniquePanda]

Fixes #10461

This adds a new boolean parameter updateImageIndex to TextureAtlas.prototype.addSubRegion() which is false by default. If it is true, the _indexHash value for the given image id is updated to point to the index of the new sub region. Otherwise the index will still point to the region of the whole image (same behavior as currently).

This is needed because billboards create such a subregion, but wouldn't currently use this subregion in later calls to _loadImage which results in the problems described in the issue.

I am not really sure if my solution is good, because I am no expert on this topic. There are some open questions:

1. Does this has any side effects, that I overlooked?
2. Should the index be updated always (without the need for the updateImageIndex parameter)?
3. Should there be a way to still access the previous region? Maybe two arrays are needed, a _indexHash and a _subRegionIndexHash?
4. addSubRegion isn't used anywhere else (only by _loadImage() of Billboard.js). Should this be refactored in general somehow?

I didn't add/update any tests yet, because of those questions.

Longer description of what I think is the problem of the current code that is fixed by this PR:
An imageSubRegion for the billboard backgrounds is added to the texture atlas on the first load of a billboard.
Later calls will not add anything to the texture atlas, but will request the correct image by relying on the texture atlas' _indexHash.
In these later calls the index is retrieved from the texture atlas and then directly passed to completeImageLoad().
This function then uses this index to get the correct texture coordinates from the texture atlas. The problem is, that the image sub region is not taken into account anymore at this point.
The initial call, that added the sub region, is getting the index by calling TextureAtlas.addSubRegion() which returns a promise that resolves to the index. This is the index of the created subregion.
However, later calls will receive the index of the whole image, because the _indexHash will still point to the index of the image and not its sub region. That's why the background looks different then.

[cesium-concierge]

Thanks for the pull request @UniquePanda!

• ✔️ Signed CLA found.
• ❔ 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]

Thanks for the fix @UniquePanda! Good catch! I think this was an oversight in #10275. When we resolve the promise in addImage, we also update the image id hash. However, that was not done for image sub regions, hence the bug.

Given this is fixing that oversight, as well as that TextureAtlas is a private class and this is the only place where that method is being used, it should be safe to make this fix the default behavior. The additional parameter to the the addImageSubRegion method` can be removed.

[UniquePanda]

I think this was an oversight in #10275

Ah yes, makes sense. I was wondering, why the problem didn't occur before and had forgotten about that PR :)

I removed the parameter and the index hash is now updated always, when a sub region is added.
I also added one line to the tests to test that behavior. However, I wasn't able to get that particular test to fail, even if I randomly changed numbers in it. So for me the tests passes locally, but it does so always, regardless if the assertions are correct or not 😅
So you can either keep the line in and trust it (if the CI passes) or someone else should try if the test fails with changed numbers 😅

[ggetz]

Another good catch @UniquePanda! That test was not properly calling the final expectations of that test. I re-wrote the problematic specs a bit so that the expectation are actually being executed. I also made a tweak to your new expectation so that we avoid calling private members of the class where possible because accessing private members makes tests more tightly coupled to the source code, and therefore more fragile.

If you could add a note to CHANGES.md mentioning the issue being fixed, and assuming my tweaks pass CI, this should be good to merge!

[UniquePanda]

That test was not properly calling the final expectations of that test

Haha, I have to admit that confused me a bit. I actually started to change the test like you did, but then thought I might just don't understand what it was doing Thanks for fixing. 😄

I updated the CHANGES.md.

[ggetz]

There's some antiquated promise behavior in some tests, so it's good to get them cleaned up. 😄 Thanks again @UniquePanda, happy to get this fix in!