Skip to content

Conversation

@kchapelier
Copy link
Contributor

This patch sets to 1 the unpackAlignment of the DataTexture3D generated by LUTCubeLoader.

There was an issue when LUTCubeLoader is used to load valid 3D .cube files with specific table sizes, the call to gl.texImage3D would fail with the following error: WebGL: INVALID_OPERATION: texImage3D: ArrayBufferView not big enough for request. I specifically noticed the issue with 2x2x2 and 3x3x3 LUT tables.

It should be noted that the issue didn't happen when using the DataTexture (2D) generated by LUTCubeLoader because that class has a default unpackAlignment of 1, unlike DataTexture3D which has a default unpackAlignment of 4. For the sake of clarity, I explicitely set the unpackAlignment to 1 for both the DataTexture and the DataTexture3D in this patch.

/fyi @gkjohnson

Thanks to twitter user KimiruHamiru for the help btw.

Example of a valid 3D .cube file which would trigger the issue :

# Header
TITLE "WhiteToYellow"

LUT_3D_SIZE 3

DOMAIN_MIN 0.0 0.0 0.0
DOMAIN_MAX 1.0 1.0 1.0

# Data table
0 0 0
0.5 0 0
1 0 0

0 0.5 0
0.5 0.5 0
1 0.5 0

0 1 0
0.5 1 0
1 1 0

0 0 0.5
0.5 0 0.5
1 0 0.5

0 0.5 0.5
0.5 0.5 0.5
1 0.5 0.5

0 1 0.5
0.5 1 0.5
1 1 0.5

0 0 1
0.5 0 1
1 0 1

0 0.5 1
0.5 0.5 1
1 0.5 1

0 1 1
0.5 1 1
1 1 0

@Mugen87 Mugen87 added this to the r128 milestone Apr 10, 2021
@gkjohnson
Copy link
Collaborator

gkjohnson commented Apr 10, 2021

I think it makes more sense to to default the DataTexture3D.unpackAlignment to 1 especially if that's what DataTexture.unpackAlignment defaults to. I don't think there's a reason the two should default to different values and 1 is definitely the more user-friendly value from my understanding of the setting. I imagine it was an simple oversight when DataTexture3D was created.

@mrdoob @Mugen87 is there a reason DataTexture3D.unpackAlignment isn't initialized to 1?

@mrdoob
Copy link
Owner

mrdoob commented Apr 10, 2021

Yeah, I think setting unpackAlignment to 1 in DataTexture3D (and DataTexture2DArray too while we're at it) makes sense.

@DavidPeicho Sounds good?

@kchapelier
Copy link
Contributor Author

@gkjohnson Yes, the difference of unpackAlignment between DataTexture and DataTexture3D caught me off-guard, but I assumed there was a reason for it, either for performance or something else.

@DavidPeicho
Copy link
Contributor

Sounds good to me!

@mrdoob
Copy link
Owner

mrdoob commented Apr 10, 2021

Done: #21633

@mrdoob mrdoob closed this Apr 10, 2021
@mrdoob mrdoob removed this from the r128 milestone Apr 10, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 10, 2021

@kchapelier Thanks for raising the issue! 🙏

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.

5 participants