Skip to content

Conversation

jniac
Copy link

@jniac jniac commented Sep 1, 2023

This is a quick fix to allow EXRExport to export cube texture as a single vertical strip (compatible with Unity).

The exporter currently crashes because no face index is specified when calling renderer.readRenderTargetPixels(...) but face index is required here to access the right "sub" framebuffer from the array.

There is still an open question:
How to dispose the data? Actually faces are processed in the following order +X, -X, +Y, -Y, +Z, -Z

In order to be compatible with Unity, the data should be flipped vertically face by face over the Y axis.

Unity preview:

cubemap-preview-unity

But, if the thumbnail looks OK in Unity, the strip looks weird otherwise:

  • Each face looks flipped also horizontally.
  • "y" faces still looks flipped vertically.

studio-env-(9)

So maybe with more documentation, a more general solution may be written.

References:
cube texture Khronos

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 2, 2023

/ping @sciecode

@Mugen87 Mugen87 changed the title fix exr exporter when exporting cube texture EXRExporter: Add support for exporting cube textures as vertical strip. Sep 2, 2023
@sciecode
Copy link
Contributor

sciecode commented Sep 2, 2023

Thank you for the ping Mugen87.

While vertical strip cubemaps aren't uncommon, even Unity allows for different cubemap layouts. This PR enforces this set configuration for all uses of cubemap RenderTargets - which maybe not be what other users want.

I considered this situation back when I first implemented the exporter, but decided it was best if users themselves handled how cubemaps should be flatten; by constructing the final RenderTarget in user-space.

If there's sufficient demand, perhaps we could implement a dedicated API to handle some of the cases. But I don't believe this proposed solution is universal enough in it's current implementation.

@sciecode
Copy link
Contributor

sciecode commented Sep 2, 2023

The exporter currently crashes because no face index is specified when calling renderer.readRenderTargetPixels(...)

However we should inform the user of this limitation when trying to read from a cubeTexture, it shouldn't just crash on them.

@jniac
Copy link
Author

jniac commented Sep 4, 2023

This PR enforces this set configuration for all uses of cubemap RenderTargets - which maybe not be what other users want.

I do agree. So for the moment a simple error with explicit message about cubeTexture may be enough.

However, i'm wondering if the EXRExporter could accept any textures, or buffers, instead of a render target as an input.

@sciecode
I'm making a tool to export environment textures, I would like to provide the common cubemap layout (equirectangular, cross, strip), and I only need from the EXRExporter the final steps (compression, file encoding), so what about another public method? Does it make sense?

Conversely, I think now that the best way to export cubemaps is to use a dedicated render target used in conjonction with a dedicated shader that will encore the pixels according to the different existing standard layout.
@Mugen87
Do you believe that an example for cubemap encoding / exporting is something that threejs may desire to add to its existing examples? (If so I may begin by a threejs style example).

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 4, 2023

Do you believe that an example for cubemap encoding / exporting is something that threejs may desire to add to its existing examples? (If so I may begin by a threejs style example).

It would be good to have an example for each type of exporter so adding misc_exporter_exr sounds fine. The example could use lil-gui to provide a simple dropdown menu for exporting different kind of texture setups. This would be the ideal place for adding a cube map example.

@sciecode
Copy link
Contributor

sciecode commented Sep 4, 2023

However, i'm wondering if the EXRExporter could accept any textures, or buffers, instead of a render target as an input.

Yes, I do believe so. Specifically a DataTexture interface would make sense to me, cause all information required by exporter is provided by default (ImageBuffer, Width/Height, Format, Type). Having the current .parse API alongside this proposed .parseDataTexture, would be my preferred way of moving forward.

Would you like to work on this PR @jniac, or you'd prefer if I did it?

@jniac jniac changed the title EXRExporter: Add support for exporting cube textures as vertical strip. EXRExporter: Add support for exporting textures (EDIT). Sep 4, 2023
@jniac
Copy link
Author

jniac commented Sep 4, 2023

I can do it myself, following the same style you used (very simple class methods calling pure functions with explicit names), then you'll check if it's ok for you. I'm wondering what could be the name of the new function.

If parse() is for RTT, should we propose parseTexture() for textures ?

This will even allows a parseCubeTexture() in the future which may return an array of 6 exr files (instead of a single strip file).

Should we rename parse() to parseRenderTarget(), keep the old name and mark it as deprecated?

@sciecode
Copy link
Contributor

sciecode commented Sep 4, 2023

I can do it myself, following the same style you used (very simple class methods calling pure functions with explicit names), then you'll check if it's ok for you.

Sure, feel free to ping and I'll review the changes.

I'm famously bad at naming APIs 😆 So perhaps @Mugen87 and others could help us with that.

My only concern with parseTexture() is that it implies any texture can be exported, but at this moment only DataTexture will be supported.

The name parse was also used based on the API provided by other exporters, which never made much sense to me - in this context. If we are to change, perhaps it would makee sense to also reconsider that.

I'm not particular about any of this, we should just try to minimize changes as to avoid confusion by users, but whatever others agree on, is fine with me.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 4, 2023

Let's avoid to question the parse*() syntax for now since I fear this will only defer or even block the PR.

I would prefer to keep EXRExporter similar to KTX2Exporter. When using render targets, KTX2Exporter provides the following API:

exporter.parse( renderer, renderTarget );

However, the exporter also supports textures like so:

exporter.parse( texture );

I would do the same for EXRExporter. Meaning you check the argument types in parse() and then execute different code paths.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 25, 2023

Now that #26810 has been merged (🎉 ), how should we proceed with this PR?

@jniac
Copy link
Author

jniac commented Sep 26, 2023

Now that #26810 has been merged (🎉 ), how should we proceed with this PR?

We can close it I think. I made an example for exporting cube texture into various formats (equirectangular + horizontal stripe for now) but it's a separate topic. So we can close this one.

Could you tell me if it is worth integrating such an example? If so, i can complete the example, clean the code and create a new PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 26, 2023

So we can close this one.

Okay!

Could you tell me if it is worth integrating such an example? If so, i can complete the example, clean the code and create a new PR.

I'm not very familiar with the EXR standard so I'm not sure how to proceed with cube map conventions. But I admit it could be useful for other devs to see how stripe or cross layouts are created. I'm okay with whatever @sciecode thinks is best.

@Mugen87 Mugen87 closed this Sep 26, 2023
@sciecode
Copy link
Contributor

OpenEXR spec defines an envmap hint attribute for equirectangular & vertical-stripe envmaps, however this attribute is mostly ignored by most loaders, and completely stripped out by all editing software that I've used. So, it should really be up to user which cubemap convention they want, as the file will be treated as plain data in almost every case.

Could you tell me if it is worth integrating such an example? If so, i can complete the example, clean the code and create a new PR.

Since we expect the user to generate their own preferred data-layout, showcasing how to generate different layouts could indeed be useful. I think we should just add cross layout to your example, as Mugen suggested.

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.

3 participants