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

RGBFormat removed from r137 #64

Open
divisuals opened this issue Mar 10, 2023 · 4 comments
Open

RGBFormat removed from r137 #64

divisuals opened this issue Mar 10, 2023 · 4 comments
Assignees

Comments

@divisuals
Copy link

Tried using recent THREE release (r149), and got the following error:

WARNING in ./node_modules/mathbox/build/esm/render/buffer/texture/datatexture.js 57:12-27 export 'RGBFormat' (imported as 'CONST') was not found in 'three' (possible exports: ACESFilmicToneMapping, ...

Per THREE r137, RGBFormat is removed:

Texture
Remove RGBFormat. mrdoob/three.js#23228, mrdoob/three.js#23201, mrdoob/three.js#23205, mrdoob/three.js#23211, mrdoob/three.js#23223, mrdoob/three.js#23218, mrdoob/three.js#23219, mrdoob/three.js#23367 (@Mugen87)

As this is only used in 1 class, a simple fix could be to just replace it with RGBAFormat, per this recommendation.

@ChristopherChudzicki ChristopherChudzicki self-assigned this Mar 10, 2023
@divisuals
Copy link
Author

divisuals commented Mar 13, 2023

Not sure if this is relevant: CONST.RGBFormat => gl.RGB lookup was removed from util/three.js in this commit - 3987ae6 (Feb/17/2022).

Also, this error is not reproducible in any tests or examples. One has to import the mathbox esm module in a different downstream project to reproduce it.

@ChristopherChudzicki
Copy link
Collaborator

@divisuals Yeah, I looked into this a bit over the weekend.

Ideally, we'd (1) understand the intent of the original RGBFormat for 3-channel, and (2) preserve that with the current version of THREEJS.

But... I tried downgrading to r136 and couldn't get 3-channel colors to work (even after restoring the line from 3987ae6).

I'm sort of inclined to just replace the current instance of RGBFormat with an explicit undefined, which would perfectly preserve the current behavior (including any associated bugs) and silence that warning.

@sritchie Does ☝️ sound reasonable to you?

@divisuals
Copy link
Author

Haven't seen any movement on this, so sharing my short-term workaround:

  • As mathbox is directly managing the data textures, I could get around compilation errors by just changing RGBFormat to RGBAFormat on this line
  • The code compiles and doesn't create any obvious runtime errors

Changing gl params to RGBA throws expected error:
texSubImage2D: ArrayBufferView not big enough for request.

@ChristopherChudzicki can you please share which 3-channel color example didn't work for you? Thanks

ChristopherChudzicki added a commit that referenced this issue May 5, 2023
RGBFormat was removed in ThreeJS r137. Replacing with explicit undefined silences some build warnings and preserves existing behavior (including any bugs)

See #64
ChristopherChudzicki added a commit that referenced this issue May 5, 2023
RGBFormat was removed in ThreeJS r137. Replacing with explicit undefined silences some build warnings and preserves existing behavior (including any bugs)

See #64
@ChristopherChudzicki
Copy link
Collaborator

Chris: I tried downgrading to r136 and couldn't get 3-channel colors to work (even after restoring the line from 3987ae6).
divisuals can you please share which 3-channel color example didn't work for you? Thanks

@divisuals I believe I was just trying a surface with colorExpr emitting 3-components instead of 4. But I'm not sure if that's what "3 channel color" was supposed to do.

Re my comment above:

I'm sort of inclined to just replace the current instance of RGBFormat with an explicit undefined, which would perfectly preserve the current behavior (including any associated bugs) and silence that warning.

I just opened a PR that does this #69; that branch is published on npm as mathbox@2.3.2-rc1 if you want to try it in your project. This change seems pretty safe, but I want to do a bit more manual testing before publishing it.

Thanks for poking us about this.

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

No branches or pull requests

2 participants