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

Bugfix: 1238833 #283

Closed
wants to merge 4 commits into from
Closed

Bugfix: 1238833 #283

wants to merge 4 commits into from

Conversation

skhiat
Copy link
Contributor

@skhiat skhiat commented Apr 27, 2020

Owner changed: @RSlysz

Bug:
https://fogbugz.unity3d.com/f/cases/1238833/
To repro: enable guizmo (check the video).

Code:
Apparently in this context it's forbidden to use multicontext (message-error).

Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Any test projects to go with this to help reviewers?


Comments to reviewers

Notes for the reviewers you have assigned.

@sebastienlagarde
Copy link
Contributor

@skhiat please add a changelog (add the entry at the end of the Fixed section (not at the beginning :) )

@RSlysz
Copy link
Collaborator

RSlysz commented May 4, 2020

Oh boy. This is a new things introduced in recent trunk it seams.
I dunno why it is not allowed anymore.
Note that we use multi-target for

  • FrameSettings (but in OnInspectorGUI)
  • ReflectionProbes
  • Lights

It is possible that decal is the only one using it in OnSceneGUI which can be a long lasting mistake.

@remi-chapelain have you tested with multiedition? Multi-target is used to compound operation on each target of the selection generally. So this can broke the multi-edition at some extend. This need a thorough check.

@@ -150,22 +150,15 @@ public Bounds OnGetFrameBounds()
public void UpdateMaterialEditor()
{
int validMaterialsCount = 0;
for (int index = 0; index < targets.Length; ++index)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this problematic loop on target.

It has no meanings on this file l.112 as it is used as a hook on each DecalProjector when the material change. So there we should not use the target notion. We should have a DecalProjector parameter to gather changes.

On line 109 though, the same function is called from OnEnable where multiselection can work on several target at a time.

It seams that this function (original version) is OK for line 109 but we should add another dedicated callback for the individual updated used in event

Copy link
Contributor Author

@skhiat skhiat May 5, 2020

Choose a reason for hiding this comment

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

After slack discussion. The problem seems to come from another direction.

@remi-chapelain remi-chapelain self-requested a review May 5, 2020 09:06
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

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

Indeed, thanks for pointing that out @RSlysz
It breaks the multi material-editing on decals.
Before the PR, when selecting multiple decal projector with different material, you could edit all materials at once in the inspector window. With this fix you can only edit one.

@RSlysz
Copy link
Collaborator

RSlysz commented May 12, 2020

Replaced by PR #413

@RSlysz RSlysz closed this May 12, 2020
@skhiat skhiat deleted the HDRP/bugfix branch November 12, 2020 12:14
PaulDemeulenaere added a commit that referenced this pull request Oct 18, 2021
* Fix incorrect condition for GPUEvent

Mistake introduced at #4457

* *Add some editorTest

* Add some test to covers corners case

See https://unity.slack.com/archives/G1BTWN88Z/p1632919330247400

* *WIP* Improve incorrect connection handling

* Fix multiple type in output of initialize part

* Fix Update to Update which is actually allowed

* *Update changelog.md

* Fix missing case for VFX.Update tyep

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

Successfully merging this pull request may close these issues.

4 participants