Skip to content

Conversation

julienamsellem
Copy link
Contributor

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1315493/

The context menu for "Contexts" in the VFX Graph had no Paste option.
Now the paste option is added and also to avoid confusion the option is disabled when we detect that the paste action is not allowed (previously there was an error message in the console)


Testing status

  • Paste block from one context to the same context or other context.
  • Paste block on the graph or an operator (not allowed)
  • Paste an operator on a context (the operator is pasted on the graph)

Comments to reviewers

Notes for the reviewers you have assigned.

@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.

@VladNeykov VladNeykov requested review from VladNeykov and removed request for a team September 9, 2021 20:08
@VladNeykov
Copy link
Contributor

VladNeykov commented Sep 10, 2021

Hi @julienamsellem ,
Thanks for the fix, it looks good! Tried pasting blocks on blocks, contexts (works), nodes and the graph itself (doesn't, as expected). Checked ShaderGraph as well to make sure the paste option doesn't propagate there.

I found a related issue which is not caused by your PR, but might be addressable here. I wonder if it's feasible to disable pasting a block subgraph within itself when opened, as it leads to a crash:
https://user-images.githubusercontent.com/7230872/132842122-52dccc0b-100f-4b74-8934-f24713deccfd.mp4

@julienamsellem
Copy link
Contributor Author

julienamsellem commented Sep 10, 2021

I found a related issue which is not caused by your PR, but might be addressable here. I wonder if it's feasible to disable pasting a block subgraph within itself when opened, as it leads to a crash:
https://user-images.githubusercontent.com/7230872/132842122-52dccc0b-100f-4b74-8934-f24713deccfd.mp4

I'd prefer to open a new issue for this case so that this PR stays "atomic" and then easier to backport or revert :)

@VladNeykov
Copy link
Contributor

I'd prefer to open a new issue for this case so that this PR stays "atomic" and then easier to backport or revert :)

Sounds good! :) In that case, no outstanding issues, I've logged the above-mentioned one here: https://fogbugz.unity3d.com/f/cases/1364480/

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.

Works as expected, no PR-specific issues found.
Thanks for the fix!

@julienamsellem julienamsellem marked this pull request as ready for review September 10, 2021 14:10
var selectedContexts = view.selection.OfType<VFXContextUI>();
var selectedBlocks = view.selection.OfType<VFXBlockUI>();

return selectedBlocks.Any() || selectedContexts.Count() == 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same check that is done when really doing the paste

else
{
Debug.LogError(m_BlockPasteError.text);
Debug.LogWarning(m_BlockPasteError.text);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a error, rather an action that is not allowed. But with my changes it should not happen anymore

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.

LGTM! 🟢

@julienamsellem julienamsellem merged commit 59bd1e7 into master Sep 16, 2021
@julienamsellem julienamsellem deleted the vfx/fix/1315493-disable-paste-if-not-allowed branch September 16, 2021 07:17
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.

3 participants