CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
13.02k stars 3.51k forks source link

Fix label rendering bug in WebGL1 contexts #12301

Closed jjhembd closed 1 week ago

jjhembd commented 1 week ago

Description

This PR fixes a texture loading bug in WebGL1 contexts.

12100 refactored the Texture class for better readability and documentation. In the process, some functions made use of the longer signature for the texImage2D and texSubImage2D methods, for pixel data supplied as either a TypedArray or an HTMLImageElement. However, only WebGL2 allows the longer signature for HTMLImageElement (or similar) data sources. This resulted in a TypeError when loading some textures in a WebGL1 context.

The errors came from the Texture.prototype.copyFrom method. As part of this fix, the loading calls for different copyFrom source types were pulled out into helper functions, for better readability and consistency with the Texture constructor.

Issue number and link

Fixes #12249

Testing plan

Load this local Sandcastle and verify that the label renders without errors.

Author checklist

github-actions[bot] commented 1 week ago

Thank you for the pull request, @jjhembd!

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

jjhembd commented 1 week ago

Thanks for the review @ggetz !

Would it be possible to add a spec to make sure this regression never happens in the future?

The existing Texture specs caught the bug, when run in WebGL1. But we have been running them in WebGL2 only.

Since the Texture class is so widely used, I think the safest route will be to test everything in both WebGL1 and WebGL2. I updated TextureSpec to do this, using the createWebglVersionHelper method from #11201.

I think I addressed all the other feedback.

ggetz commented 1 week ago

Thanks for the updates @jjhembd! I like the plan to run with both WebGL 1 and 2.

I'm seeing some hangups and unexpected failures when running the tests locally. Are you seeing anything similar?

jjhembd commented 1 week ago

I'm seeing some hangups and unexpected failures when running the tests locally. Are you seeing anything similar?

Yes, sorry, that was an oversight from https://github.com/CesiumGS/cesium/pull/12301/commits/d385f34702edd1a52b128cb84b2f4a1187f72257. Should be fixed now!

ggetz commented 1 week ago

Awesome, this is now working for me! Thanks @jjhembd!