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

Remove tonemapping from MeshBasicMaterial #9603

Closed
4 of 12 tasks
bhouston opened this issue Aug 30, 2016 · 11 comments · Fixed by #17307
Closed
4 of 12 tasks

Remove tonemapping from MeshBasicMaterial #9603

bhouston opened this issue Aug 30, 2016 · 11 comments · Fixed by #17307

Comments

@bhouston
Copy link
Contributor

Description of the problem

We ran into this issue with Clara.io. We want to be able to show specific color widgets for manipulation, while on top of a Tone Mapped scene. But we do not want these specific color widgets to be tone mapped themselves. Currently the tone mapping is applied on the MeshBasicMaterial in this line here:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderLib/meshbasic_frag.glsl#L52

I would like to remove this.

I believe this is justified because MeshBasicMaterial is actually an unlit material thus tone mapping is sort of illogical on this material anyhow.

This will affect how basic materials look, but I believe it is the correct choice.

This is especially a problem on dark scenes where one wants to increase the exposure but it will then bleach out the manipulator/widgets.

Three.js version
  • Dev
  • r80
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • Linux
  • Android
  • IOS
Hardware Requirements (graphics card, VR Device, ...)
@bhouston
Copy link
Contributor Author

The other alternative is to make tonemapping optional on materials via a define that is controled by a per-material toneMapping = true boolean. We can do that. I am not sure it is needed because MeshBasicMaterial is actually an unlit material, it has an explicit color that should just be respected -- or at least that is my interpretation of it.

@mrdoob
Copy link
Owner

mrdoob commented Aug 31, 2016

The other alternative is to make tonemapping optional on materials via a define that is controled by a per-material toneMapping = true boolean. We can do that.

I think that sounds good.

Otherwise, I can easily imagine the situation where someone is using MeshBasicMaterial in a lamp and tone mapping the whole scene. It would be weird if suddenly the lamp didn't matched with the rest of the scene.

@JustASquid
Copy link

I'm in the same boat now with UI elements using MeshBasicMaterial that are being tonemapped. Was a fix or workaround ever put into place?

@WestLangley
Copy link
Collaborator

@bhouston Is this still an issue for you?

We decided early-on that tonemapping should be set at the renderer level uniformly across all materials. Is material-specific tonemapping desired now? Seems like overkill to me.

Yet, having a per-material on-off switch seems odd to me.

Removing tonemapping from MeshBasicMaterial is an option, as you suggested.

@WestLangley WestLangley changed the title Remove tonemapping from MEshBasicMaterial Remove tonemapping from MeshBasicMaterial Aug 20, 2019
@bhouston
Copy link
Contributor Author

I think if there is consistent demand then we make an option called .toneMapping =true/false on materials that allows you to opt out.

@bhouston
Copy link
Contributor Author

It all depends on whether you are trying to match an overlay color or an existing color in the scene directly. If you are trying to match an existing color in the scene, then you want it tone mapped, if you are trying to map an overlap color, then you want it to not be tone mapped. Thus I see both use cases here.

@WestLangley
Copy link
Collaborator

@bhouston I do not have a problem adding such a feature. It is the API that I have a problem with.

renderer.toneMapping is a type of tone mapping;
I don't think material.toneMapping should be a boolean.

Maybe this?

material.toneMapped = false; // default true

I assume it would be a property of Material, so all materials would have this feature?

@bhouston
Copy link
Contributor Author

Sorry, I undercommunicated. I agree, it should be material.toneMapping = true/false to allow a material to opt-in or not of tone mapping. We are on the same page.

@WestLangley
Copy link
Collaborator

@bhouston ... but it doesn't seem to me you agreed with what I wrote... ?

@mrdoob
Copy link
Owner

mrdoob commented Aug 21, 2019

material.toneMapped = false; // default true

That sounds good to me.

@WestLangley
Copy link
Collaborator

OK. Will do.

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

Successfully merging a pull request may close this issue.

4 participants