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

support all texture formats and types in DataTextureArray.addLayerUpdate and CompressedTextureArray.addLayerUpdate #28654

Merged
merged 19 commits into from
Jun 20, 2024

Conversation

HunterLarco
Copy link
Contributor

@HunterLarco HunterLarco commented Jun 13, 2024

Description

In #27972 I added logic which allows three.js clients to update specific layers within a texture array and in doing so authored this code which determines the number of bytes required to represent a single layer within a texture array.

This code is accurate for many format-type pairs, but not all. In this PR I updated the logic to include all format-type pairs currently supported by THREE. The impact is that setLayerUpdate will now be accessible to more projects.

Design Notes

I've written this PR so that TextureUtils.getByteLength is exposed as a helper to clients. My company often works with texture arrays and would get a lot of value by having this method available. It's frequently helpful when managing memory to know how large a layer is. Knowing this, I feel it's a good helper to expose.

This contribution is funded by SOOT

Copy link

github-actions bot commented Jun 13, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
679.4 kB (168.3 kB) 681.3 kB (168.7 kB) +1.83 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
457.4 kB (110.4 kB) 459.4 kB (110.9 kB) +1.99 kB

@HunterLarco HunterLarco marked this pull request as ready for review June 13, 2024 23:52
@HunterLarco
Copy link
Contributor Author

It seems like the provided example is failing just on windows CI=3. I'm not familiar with the difference between CI 0, 1, 2, and 3. I'd appreciate some guidance on what next steps to take and why CI=3 would fail while [0-2] succeeded.

@RenaudRohlinger
Copy link
Collaborator

I tried to simplify the example as much as possible to fix the CI, but it seems that Windows might behave differently than macOS on this.

The CI running under Windows generates a diff of 2.3%:

Diff wrong in 2.3% of pixels in file: webgl_texture2darray_layerupdate

Did you also generate the screenshot under macOS? If so, could you try to run it on a Windows see if anything behave differently, and if not maybe try to generate the example on it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 16, 2024

If you have troubles with this particular example, let's put on the exception list for now:

const exceptionList = [

@HunterLarco
Copy link
Contributor Author

Fixed! I realized that the use of setInterval in the example was causing flakiness in CI. To resolve this, the example has been modified so that the controls GUI can be used to manually move textures from a source array into a dest array. In fact this is nice because it allows for even more control/experimentation with the feature.

Screen.Recording.2024-06-18.at.12.03.11.PM.mov

@HunterLarco
Copy link
Contributor Author

Would it be possible to get this in for r166? Until landed, ETC2 formats cannot be used with addLayerUpdate which is unfortunate because it means that iOS and Safari are both forced into less performant texture array updates. It would be nice to catch the next release so that my company can improve our rendering for Safari + mobile users.

* Given the width, height, format, and type of a texture. Determines how many
* bytes must be used to represent the texture.
*/
function getByteLength( width, height, format, type ) {
Copy link
Collaborator

@Mugen87 Mugen87 Jun 20, 2024

Choose a reason for hiding this comment

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

Can this function be internal or is it required for the user to have access to it?

If possible, I would prefer to not expose it and move the logic to WebGLUtils instead. In a different PR, there is the plan to add TextureUtils as an addon in examples/jsm (see #28512 (comment)). It is not ideal to have the same module in the core and as an addon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait, I see getByteLength() is used in the new example...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is an advanced feature, I would probably not expose the helper for now and inline the size computation in the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think addLayerUpdate has limited value if three doesn't provide clients a way to know the byte bounds of a single layer. You need TextureUtils.getByteLength to know which subarray within the CompressedArrayTexture's data to update. We could make this method internal, but then it would force clients to either implement this logic themselves or copy/paste it from the three repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And inlining would fail in the example because the KTX2Loader dynamically selects a format + type depending on the underlying WebGL context. There's no guarantee that the hard-coded byte length will be consistent on different devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(also thanks for the quick review)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's see how we can organize the code regarding #28512.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code in #28512 is so compact so I think we can move it in src as well. That means there is no need to update this PR.

It's definitely good that the layer size computation in uploadTexture() is moved into a helper function.

@Mugen87 Mugen87 added this to the r166 milestone Jun 20, 2024
@Mugen87 Mugen87 merged commit 9c00c1c into mrdoob:dev Jun 20, 2024
12 checks passed
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 20, 2024

The example is unfortunately not in an ideal state since a black screen by default isn't good. Instead of using a lil-gui, what do you think about cycling through the layers (1, 2, 3) in a fixed interval?

If the E2E test fails, just add it to the exception list.

@HunterLarco
Copy link
Contributor Author

The example is unfortunately not in an ideal state since a black screen by default isn't good. Instead of using a lil-gui, what do you think about cycling through the layers (1, 2, 3) in a fixed interval?

Sure! That was the original example earlier in this PR, I can put up another change to revert to that version of the example (with an exception in E2E testing).

Alternatively, I could just initialize the example by writing the first 3 layers to the screen and then letting users navigate the dat.gui. Do you have a preference?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 20, 2024

Alternatively, I could just initialize the example by writing the first 3 layers to the screen and then letting users navigate the dat.gui. Do you have a preference?

Let's use this approach since webgl_texture2darray_compressed is already animated.

@Mugen87 Mugen87 changed the title support all texture formats and types in DataTextureArray.setLayerUpdate and CompressedTextureArray.setLayerUpdate support all texture formats and types in DataTextureArray.addLayerUpdate and CompressedTextureArray.addLayerUpdate Jun 23, 2024
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