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

Textures: Fix clearing of textures with integer type #14112

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

Popov72
Copy link
Contributor

@Popov72 Popov72 commented Aug 1, 2023

See https://forum.babylonjs.com/t/support-custom-outputs-for-shadermaterial/42835/3

It also fixes a couple of small bugs with WebGPU.

PG for WebGL: https://playground.babylonjs.com/#XSNYAU#47
PG for WebGPU: https://playground.babylonjs.com/#XSNYAU#50

WebGPU has a stricter limitation for the total number of components for the target render textures (it's limited to 32 bytes), so I had to make a different PG.

@bjsplat
Copy link
Collaborator

bjsplat commented Aug 1, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Aug 1, 2023

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Sounds expensive and creating GC ? should we always call clearColor and clearBuffer ? But only on color change ?

@Popov72
Copy link
Contributor Author

Popov72 commented Aug 2, 2023

I have removed the GC. I'm not sure to understand this part:

should we always call clearColor and clearBuffer ? But only on color change ?

?

@sebavan
Copy link
Member

sebavan commented Aug 2, 2023

I have removed the GC. I'm not sure to understand this part:

should we always call clearColor and clearBuffer ? But only on color change ?

?

To limit the branching within the function, as well as the gl calls, I was wondering whether we should check the current state (value wise) and always call clearColor… in this case so we end up with only one if preventing extra gl calls instead of the one branching on type. I am not sure it is necessary tbh :-)

@Popov72
Copy link
Contributor Author

Popov72 commented Aug 2, 2023

I don't think there are currently extra gl calls(?)

We could call clearColor even in the integer texture case, but we will still have a condition because we should not update mode if we called clearBuffer previously.

Note that clearBuffer actually clears the texture (contrary to clearColor which only updates the gl state), so we must call it each time ThinEngine.clear is called, and not only when the color changes.

We could make the test for an integer texture lighter if we add a getter property in InternalTexture like isIntegerTexture that would be precomputed when the format is set (but we would need to transform the InternalTexture.format property to a getter/setter...).

@deltakosh deltakosh merged commit b0817ee into BabylonJS:master Aug 3, 2023
9 checks passed
@sebavan
Copy link
Member

sebavan commented Aug 3, 2023

I don't think there are currently extra gl calls(?)

We could call clearColor even in the integer texture case, but we will still have a condition because we should not update mode if we called clearBuffer previously.

Note that clearBuffer actually clears the texture (contrary to clearColor which only updates the gl state), so we must call it each time ThinEngine.clear is called, and not only when the color changes.

We could make the test for an integer texture lighter if we add a getter property in InternalTexture like isIntegerTexture that would be precomputed when the format is set (but we would need to transform the InternalTexture.format property to a getter/setter...).

Makes sense ! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants