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

Vfx/fix/1359426 shift context on new block #5578

Merged
merged 7 commits into from
Oct 4, 2021

Conversation

julienamsellem
Copy link
Contributor

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1359426/
When inserting a new node in a context the context could then overlap the context below.
With the fix, each context below is pushed down to avoid overlapping (it's not moved if not needed).
[Before]
Before

[After]
After


Testing status

I tested inserting new nodes with the filter window or with copy/paste an existing node.
I tested Spawn, Init and Update contexts.


Comments to reviewers

If the contexts are side by side the behavior is the same even if it's not absolutely required the graph is still more readable if contexts are pushed down.

@github-actions
Copy link

github-actions bot commented Sep 9, 2021

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

VFX
/.yamato%252Fall-vfx.yml%2523PR_VFX_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@VitaVFX VitaVFX requested review from VitaVFX and removed request for a team September 9, 2021 16:39
@VitaVFX
Copy link

VitaVFX commented Sep 10, 2021

Looking solid, but one question: When having non-linear layout, contexts shift instead of staying where they are (attached gif as a showcase). Is there any way to identify if nothing is behind (for example there's no Update context below Init) so contexts would remain in the same location when having non-linear layout?
5tXiPWat3g

Blocks/contexts shift when and are clearly visible when:

  • Adding blocks manually to spawn, init, update, output contexts
  • Copy/pasting single and multiple blocks
  • Having disconnected systems
  • In Subgraphs

@julienamsellem
Copy link
Contributor Author

julienamsellem commented Sep 10, 2021

I changed a bit the behavior so that contexts are not shifted if they are side by side.
Until the approach is done differently there will still be some corner cases with unexpected behavior.
Today we try to adjust contexts by following the connections of the context that has increased.
But a more general approach could be a global graph analysis and fix any overlapping context/node.
But even that solution is not ideal because we should not change the layout too much in the risk of breaking the user's own graph organisation.

On edge case I found with the current fix is if two contexts are swapped vertically (output above update for example).

@julienamsellem julienamsellem marked this pull request as ready for review September 16, 2021 14:16
@VladNeykov VladNeykov self-requested a review September 21, 2021 14:34
Copy link
Contributor

@VladNeykov VladNeykov left a comment

Choose a reason for hiding this comment

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

Looking solid! Tested:

  • Adding, duplicating, copy-pasting, and moving single or groups of blocks
  • Moving blocks to previous or next contexts
  • Outputs connected to Update and Outputs connected directly to Initialize
  • Multiple outputs (only those under the previous context will move down)
  • Smoke-tested subgraph blocks added to contexts, as well as within the subgraph itself
  • Different arrangements/placement of contexts when doing the above
  • Different default templates
  • Undo/Redo

Thanks for the fix! :)

@VladNeykov VladNeykov removed the request for review from VitaVFX September 24, 2021 14:52
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere left a comment

Choose a reason for hiding this comment

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

Side note, internal on GrowContext was actually already implicit
Otherwise, the code looks good and it's clear.
Thanks for this improvement. 🟢

@julienamsellem julienamsellem merged commit f0c1049 into master Oct 4, 2021
@julienamsellem julienamsellem deleted the vfx/fix/1359426-shift-context-on-new-block branch October 4, 2021 11:56
julienamsellem added a commit that referenced this pull request Dec 1, 2021
* Automatically offset contexts when a new node is inserted

* Updated changelog

* Do not shift down contexts if they are side by side

* Do not shift a context if it's completely above the context that precede itself

Also fixed an issue increasing the shift along the contexts

* Improved context position shifting when there are multiple outgoing links

* Implemented a different approach to shift contexts observing all contexts
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
julienamsellem added a commit that referenced this pull request Dec 2, 2021
* Automatically offset contexts when a new node is inserted

* Updated changelog

* Do not shift down contexts if they are side by side

* Do not shift a context if it's completely above the context that precede itself

Also fixed an issue increasing the shift along the contexts

* Improved context position shifting when there are multiple outgoing links

* Implemented a different approach to shift contexts observing all contexts
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
@PaulDemeulenaere
Copy link
Contributor

Has been backported in #6444, updating flag

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