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

Fixed multi-editing with new light intensity slider #2600

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Nov 12, 2020

Purpose of this PR

Fix for https://fogbugz.unity3d.com/f/cases/1290191/

I am not sure why this apply was needed, i didn't find a case in which undo redo would not work without it.
A function higher in the callstack is already doing the Apply inside an EndChangeCheck test


Testing status

Tested the repro steps with the light explorer (which is not actually related).
and undo redo when editing and multi editing lights with various units
Tested undo redo when changing light unit
Tested changing light value with presets

Copy link
Contributor

@johnpars johnpars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this Apply was originally added to ensure proper Undo/Redo when switching between light unit types.

Example:

  • Create a new punctual light
  • Switch from Lumen -> Candela -> EV100
  • Undo

You will see that the undo apply in master, but not this branch.

Ideally, we should find a way to fix this regression for multi-editing, and also ensure that this undo functionality still works.

@adrien-de-tocqueville
Copy link
Contributor Author

adrien-de-tocqueville commented Nov 12, 2020

You're right, I didn't test that, thanks for the feedback.
It's not a pretty solution, but I managed to fix both behaviors by only doing the unit conversion in a local variable and write it back only when changed

Copy link
Contributor

@johnpars johnpars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - While it's unfortunate that we simply can't turn the light unit slider into a pure view of the data (IE ditch the SerializedProperty completely and simply refer to the float value), it's necessary due to how the context menu works right now.

@sebastienlagarde sebastienlagarde merged commit 96b49f5 into hd/bugfix Nov 19, 2020
@sebastienlagarde sebastienlagarde deleted the hd/fix-light-multi-editing branch November 19, 2020 10:39
sebastienlagarde added a commit that referenced this pull request Nov 23, 2020
* Update 1900_AlphaTestSG.renderTexture (#2608)

* update GBuffer description.

* Added a warning when trying to bake with static lighting being in an invalid state. (#2597)

* Added a warning when trying to bake with static lighting being in an invalid state.

* Update changelog.

* [HDRP] Fix runtime debug UI showing wrong entry (#2595)

* Redraw debug before re-registring the debugs

* changelog

* Fixed multi-editing with new light intensity slider (#2600)

* Fixed multi-editing with new light intensity slider

* Only update floatValue when changed

* Fixed useless editor repaint when using lod bias (#2601)

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fix issue with density volume gizmo editing (#2603)

* Fix

* changelog

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* [HDRP][Compositor] Fix issues with image layers (#2606)

* Empty image layers in the graphics compositor don't render black anymore

* Improve camera management in compositor

* Revert "Improve camera management in compositor"

This reverts commit cc3971d.

* Update changelog

* For image layers that are first, still clear to black if no texture is attached

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fixed issue when decal projector material is none (#2615)

* Fixed angle fading when rotating decal projector (#2618)

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fix issue with gameview repaint in the graphics compositor (#2617)

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Remove cyanish color shifting during sharpening in YCoCg in TAA (#2627)

* Do sharpening in RGB space.

* Fix alpha case

* Oops

* Ref update aside OSX

* OSX refs

* More refs

* Missing refs.

* Did I get the wrong ref?

* Fixed the sampling of the normal buffer in the the forward transparent pass. (#2643)

* Fixed the sampling of the normal buffer in the the forward transparent pass.

* Update changelog

* Update CHANGELOG.md

Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com>

* Fix custom pass reorder issue (#2609)

* Fix custom pass reorder issue

* Fix bad comment

* Refactor

* Fix bloom prefiltering tooltip (#2715)

* update reference screenshot for test 1900_AlphaTest_SG

* Fix error when loading new scene (#2713)

* Fix error when loading new scene

* Update CHANGELOG.md

* AxF Fix missing alpha blend state properties and update stencil prop (#2705)

* AxF: Fix missing alpha blend state properties and update stencil properties.

* Add changelog entry.

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fix custom pass normal and template (#2716)

* Fix custom pass template not compiling

* Fix custom pass normal buffer not bound

* Updated changelog

* Update CHANGELOG.md

Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com>

* [HDRP][Compositor] Fix edge cases when cameras are missing or deleted  (#2700)

* Fix warning spam related to cameras in compositor

* Fix null reference in AOVs

* Fix null reference when disabling camera in the compositor

* Reset target texture when disabling the compositor

* Update changelog + fix warning when output camera is deleted

* Fix issue when adding image layer without any cameras in the scene

* Fix error when disabling layer with no camera

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Update CHANGELOG.md

Co-authored-by: JulienIgnace-Unity <julien@unity3d.com>
Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com>
Co-authored-by: Antoine Lelievre <antoinel@unity3d.com>
Co-authored-by: slunity <37302815+slunity@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants