-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: add tonemapping effect #148
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @damienmontastier for your PR. Very helpful!
src/core/pmndrs/ToneMapping.vue
Outdated
const props = withDefaults( | ||
defineProps<ToneMappingProps>(), | ||
{ | ||
mode: ToneMappingMode.AGX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We set defaults only in very rare cases to minimize maintenance effort when postprocessing changes the internals of their effects. I suggest omitting these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified at next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults are still in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's done
docs/guide/pmndrs/tonemapping.md
Outdated
|
||
The `<ToneMapping>` component is easy to set up and comes with multiple tone mapping modes to suit different visual requirements. Below is an example of how to use it in a Vue application. | ||
|
||
```vue{2,4,7-8,32-36} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: While I like to have a full example on the page, I think it is better to stay uniform with the other effects and only show the usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a shame that we don't produce slightly advanced demos to present concrete use cases.
What do you think @alvarosabu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The downside would be, that there can be increased maintenance effort in case the parameters of the effects change upstream.
I'm fine as long as the docs are uniform.
Hi @damienmontastier, we will need to update this PR (and probably all the others) to adapt to the breaking change introduced in v2 https://github.com/Tresjs/post-processing/releases/tag/2.0.0 Mostly adding the correct Suffix to the effects if they are from the pmndrs/postprocessing package. Let me know if you have any questions or we can help you out. Sorry about this |
@alvarosabu I've just adapted the ToneMapping component for V2 of the package ✌️ |
This component introduces the
ToneMapping
effect, which provides an abstraction for various tone mapping algorithms from thepmndrs/postprocessing
package. The<ToneMapping>
effect is specifically designed to improve the visual rendering of HDR content by mapping high-range brightness values to a displayable range, thus enhancing luminance and color balance in the scene.EffectComposer
is added by applying tone mapping manually as an effect.<EffectComposer>
pipeline.For more details, see ToneMapping on pmndrs/postprocessing.
Local Playground —
pnpm run playground
Local Documentation —
pnpm run docs:dev
@alvarosabu @Tinoooo