Skip to content

Inspector Bugfixes #557

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

Closed
wants to merge 12 commits into from
Closed

Inspector Bugfixes #557

wants to merge 12 commits into from

Conversation

Nightmask3
Copy link
Contributor

@Nightmask3 Nightmask3 commented May 19, 2020

Purpose of this PR

The Internal Inspector was introduced in this PR: #173. Since then I've continued to perform testing in order to find issues with the Inspector and verify the parts of the shader graph workflow that must now go through the inspector instead of the cog wheel menus and the blackboard rollouts. This PR contains fixes for a number of issues both reported by users/QA, and issues found by self-testing.

Capture

This PR includes fixes for:

  • Merged Alex's PR that was lost when replacing blackboard property editing with editing through the inspector: [Shader Graph] Fix Bug with Enum Keyword Reference Suffix #58
  • Window docking issues (resizing the graph window would not move the inspector window with it)
  • https://fogbugz.unity3d.com/f/cases/1244134/
  • Can't move scroll bar with mouse because resizing handle gets in the way for the right and bottom scroll bars
  • Remove precision from top toolbar as its in graph settings now, added button called "Graph Settings" which contains it instead (this is in preparation for the move to master stacks which expose many more graph level settings)
  • Fixed the issue where clicking anywhere in the inspector that’s not a control defocuses the current node and reverts the inspector contents to the graph’s view
  • Support for custom function and subgraph nodes
  • Remove cog wheel from all kind of nodes except master nodes
  • Undo selection does not update inspector
  • Fix for Custom function/Subgraph node when input is added/removed - reorderable list header no longer goes to "IList"
  • Subgraph node property drawers work correctly now
  • Added support for VirtualTexture properties and SampleVirtualTextureNodes to inspector

Testing status

Manual Tests

  • Ran the local ShaderGraph test project and verified all tests are passing
  • Closing and opening the graph to verify serialization of layout and window sizing/positions are preserved
  • Adding/removing/renaming/moving/deleting blackboard properties and keywords along with undo/redo at every stage, verified that no regressions here
  • Changing property/keyword values through the inspector and watching the graph preview update and the shaders recompile, everything seems to be working as expected
  • Selecting graph elements (like nodes and properties from the blackboards) and verifying that the inspector displays what is selected.
  • Selecting various kinds of elements and making sure that whenever they are deselected, the inspector is hidden when nothing remains to display, including when undo/redo changes active selection.
  • When color mode is set to "Precision" and the precision is changed through the graph settings in the inspector, verified that the entire graph colors update to Float/Half if the nodes are set to Inherit.

Comments to reviewers

  • This PR removes the cog wheel from all the different kinds of nodes except master nodes, which still retain the cog wheel menus. This is because the master nodes will go away soon (approximately June 1st) and it didn't make sense to do any work to port the master node settings over to the inspector as that code would be thrown away soon regardless.
  • The reason why this touches Universal and HDRP code is because of the removal of the IHasSettings interface (which doesn't need to exist in a world with the inspector and property drawers).
    If this is an issue I can add the code for them back in but the changes are fairly straightforward (i.e. the removal of the interface reference from all master nodes) and the functionality has been preserved. Eventually this code will be removed when master stacks lands regardless so the IHasSettings interface would need to be removed at some point.
  • There's no changelog as its mostly fixing issues with the inspector and bringing it up to release quality, please let me (or Alex Lindman if I'm out) if that will be needed.
  • I will be out and unable to work past the 21st of May due to visa issues, so if anything comes up please reach out to @alindmanUnity who is helping shepherd the PR (and its backport here) to being merged while I am out.

# Conflicts:
#	com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs
# Conflicts:
#	com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs
#	com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs
…anup

[skip ci]

# Conflicts:
#	com.unity.shadergraph/Editor/Drawing/Inspector/PropertyDrawers/ShaderGUIOverridePropertyDrawer.cs
#	com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs
#	com.unity.shadergraph/Editor/Drawing/Views/MaterialNodeView.cs
…tor after deletion

# Conflicts:
#	com.unity.shadergraph/Editor/Drawing/Inspector/PropertyDrawers/ShaderInputPropertyDrawer.cs
# Conflicts:
#	com.unity.shadergraph/Editor/Data/Nodes/AbstractMaterialNode.cs
#	com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs
…ctor

# Conflicts:
#	com.unity.shadergraph/Editor/Drawing/Views/GraphEditorView.cs
#	com.unity.shadergraph/Editor/Drawing/Views/MaterialNodeView.cs
#	com.unity.shadergraph/Editor/Drawing/Views/PropertyNodeView.cs
ghost
ghost previously requested changes May 20, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

snip, mistake

@ghost ghost dismissed their stale review May 20, 2020 17:31

Accidental request changes

@ghost ghost self-requested a review May 20, 2020 17:31
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The bottom scroll bar has the same problem as the side scrollbar, scrolling with it is impossible because the whole thing gives you a resize cursor

@ghost ghost self-requested a review May 21, 2020 16:32
@marctem
Copy link
Contributor

marctem commented May 28, 2020

This branch was already merged into sg/master-stack and so this PR is redundant.

@marctem marctem closed this May 28, 2020
@marctem marctem reopened this Jun 5, 2020
@marctem
Copy link
Contributor

marctem commented Jun 7, 2020

The bottom scroll bar has the same problem as the side scrollbar, scrolling with it is impossible because the whole thing gives you a resize cursor

I see half of it giving you a resize cursor (which is annoying) but not the whole thing anymore. Please approve @LandonTownsendUnity. This needs to go in since we're not including Stacks in the first 10.0-preview drop.

@sebastienlagarde sebastienlagarde deleted the sg/inspector-bugfixes branch September 1, 2021 10:22
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