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

Bug Report - Editor Inspector Full Loads with every value change. Resetting the position of the window. #17880

Closed
GaianHelmers opened this issue May 6, 2024 · 5 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/content Categorizes an issue or PR as relevant to SIG Content.

Comments

@GaianHelmers
Copy link

Describe the bug
When editing inspector values on an entity that has enough components to cause a scroll. The window refreshes, resetting the window scroll position, and driving a long load time depending on how complicated the entity is.

Assets required
An entity with enough components to cause a window scroll in the inspector.

Steps to reproduce
Steps to reproduce the behavior:

  1. Select the default Shader Ball entity in a default level.
  2. Add script graph components until the inspector window needs to scroll.
  3. Change a value on the shader ball mesh component.
  4. Entire window reloads and resets position.

Expected behavior
The variable changes and then you're able to continue editing.

Actual behavior
The editor hangs for a moment, resets the entire inspector window, deselects the last variable, repositions the inspector either to the max or minimum scroll, then finishes hanging and allows editing again.
The issue seems to compound with how complex the entity being inspected is.

Screenshots/Video
Big Entity Edit, causes big hang and inspector reload.
https://github.com/o3de/o3de/assets/55327922/a5d52b42-8a3e-4c21-b1f0-841f053c74f6

Small Entity Edit, repositions scroll.
https://github.com/o3de/o3de/assets/55327922/69adb3e1-f159-4b5a-b2b3-20ad432a416c

Found in Branch
Dev - 9292148
But it's been around for a while.

Desktop/Device (please complete the following information):

  • Device: Desktop
  • OS: Windows 10
  • CPU Intel(R) Core(TM) i7-10700K CPU @ 3.80GHz 3.79 GHz
  • GPU MSI 3070
  • Memory 64GB
@GaianHelmers GaianHelmers added kind/bug Categorizes issue or PR as related to a bug. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 6, 2024
@michalpelka michalpelka added sig/content Categorizes an issue or PR as relevant to SIG Content. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 9, 2024
@nick-l-o3de
Copy link
Contributor

I'm still nailing this down a bit, but I have made some progress here.

There seems to be a few things at play

  1. Some components call refresh_full_newcontent needlessly (instead of refresh_full). Newcontent is supposed to be used to scroll to the bottom of the entire thing, for when a new component is created
  2. There is no mechanism to actually remember and restore the scroll when not scrolling to new content.
  3. There is no way for a component to indicate that only itself needs a full refresh, not every other component on the entire entity.

I've fixed 1 and 2 and will have a PR up soon but 3 will be a problem.

@nick-l-o3de
Copy link
Contributor

note that I can repro some of this, but I can't repro the case where changing a script canvas var causes the shuffle, even on a huge entity with all sorts of stuff on it. Other things do it, things that invoke the full refresh - for example, changing material properties or LOD properties on a mesh causes the full refresh, becuase it wants to rebuild the properties grid as they may change if this happens. However, again, if we could solve (3) above it could be a lot faster.

@GaianHelmers
Copy link
Author

GaianHelmers commented May 9, 2024

A thought on why it's not full-refeshing from script graph variable changes is:

  1. Maybe there's something going on when you have unused public variables with the used public variables present on a script graph component.

  2. Maybe after you've done iterations on a script graph, where it's shuffling used variable references to unused, and hiding public variables made private, etc. is causing it to have lingering data that drives it to have to full load?

This is purely based on intuition though. I do know that the inspector struggles when it comes to re-referencing variable changes from graph edits. Keeping data from the old variable to the new variable and causing type errors and issues processing the variables until editor reload.

@nick-l-o3de
Copy link
Contributor

nick-l-o3de commented May 10, 2024

Well I put a breakpoint in full refresh and the only place it full refreshes is when the script is recompiled which requires editing the actual .scriptcanvas file and changing it. I can't get the tree to effect a full refresh no matter what I seem to do from the variable editor in the inspector view. However, there are plenty of OTHER components that changing any value cause it to ask for a full refresh (mesh component, material component, stuff with LODs, adding a new value...) ---- its quite possible that one of them responds to a change notify by requesting a full refresh or something weird...

I think, to start this off, I'll add a new API to ask for a component-entity update only (like, make it so you CAN, as a compnent say, "I need to refresh MY entire tree" instead of "I need to refresh THE entire tree including all other components" and use it on the common ones I spot that definitely don't need to mess with the other properties of other components.

I'll also fix the scroll issue by making it at least scroll to where it was after full rebuild.

@nick-l-o3de
Copy link
Contributor

I believe this is fixed or improved by #17938.
Please retest and see if its good enough now to focus on other things...

Note: Fixed in the stabilization and the development branch, slated for next release 24/09

reopen if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/content Categorizes an issue or PR as relevant to SIG Content.
Projects
None yet
Development

No branches or pull requests

3 participants