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

Rework the surface upgrade tool to inform users without blocking #85222

Merged
merged 1 commit into from Nov 22, 2023

Conversation

YuriSizov
Copy link
Contributor

This removes the immediate confirmation dialog and insteads prints the message to the editor log (and it also appears as a toast). The immediate dialog is a devil's plaything, and it cannot be used in this scenario (if it can be used anywhere at all). The condition that triggers the SUT can happen during any attempt by the rendering server to read a mesh. This means it will conflict with a number of editor processes, like loading, importing, preview generation, export, CLI mode, etc.

So while this is less on the nose as far as informing users goes, it's also our best option to use the log and the toaster.

Closes #85202.
Closes #85201.
Likely closes #84732.

I will test a bunch of previously affected projects to see if there are no regressions. In the meantime, I'd appreciate if others double-checked what they can.

@akien-mga
Copy link
Member

Tested on the GDQuest TPS demo, it seems to work well. Testing procedure:

  • Reset repo
  • Import in 4.1.3
  • Open in 4.2-rc with this PR merged

There's a slight UX issue with the toaster, as it only shows a small part of the message which doesn't give enough info. The Output panel is not expanded by default, so it's not obvious what the user should do to understand that message:

image

We don't really have time to rework the toaster itself, but maybe we could push a different message to the toaster (shorter, referring to Output dock) and to the Output dock (all details)?

@YuriSizov
Copy link
Contributor Author

We don't really have time to rework the toaster itself, but maybe we could push a different message to the toaster (shorter, referring to Output dock) and to the Output dock (all details)?

I think we can, I'll take a look.

@Calinou
Copy link
Member

Calinou commented Nov 22, 2023

Alternatively, can the toast message be wrapped over several lines (and be trimmed much later for really long messages)?

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected on:

All these projects were imported with 4.1.3 beforehand.

I notice unknown UID errors during the upgrade process though:

WARNING: res://stage/meshes/trunk_floor.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/trunk.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/trunk_deco.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/tree_top.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/ceiling_corner.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/ceiling.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://stage/meshes/corner.res: In external resource #0, invalid UID: uid://7qqlq0kheobb - using text path instead: res://stage/tile_material.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://player/player.glb: In external resource #0, invalid UID: uid://cmvnecqga07e5 - using text path instead: res://player/player_gray.tres
     at: open (./core/io/resource_format_binary.cpp:1056)
WARNING: res://player/player.glb: In external resource #1, invalid UID: uid://be3ssftlblkjn - using text path instead: res://player/player_glow.tres
     at: open (./core/io/resource_format_binary.cpp:1056)

These don't appear after closing and reopening the upgraded project.

This removes the immediate confirmation dialog and insteads prints the
message to the editor log (and it also appears as a toast). The immediate dialog
is a devil's plaything, and it cannot be used in this scenario (if it can be used
anywhere at all). The condition that triggers the SUT can happen during any
attempt by the rendering server to read a mesh. This means it will conflict
with a number of editor processes, like loading, importing, preview
generation, export, CLI mode, etc.

So while this is less on the nose as far as informing users goes, it's also
our best option to use the log and the toaster.
@YuriSizov
Copy link
Contributor Author

Alternatively, can the toast message be wrapped over several lines (and be trimmed much later for really long messages)?

Toasts respect hardcoded newlines, but don't do wrapping. I wouldn't want to get into that right now. IIRC there were cascading sizing issues in the toasts specifically.


I've split the message into one that goes into the log and one that's displayed as a toast. Hope it's better.

I also removed suppression when exporting because we don't need it anymore, and it was actually suppressing a bit too hard. Basically, warnings will appear as often as before if you clicked the "Update only" button.

I checked Kenney's starter project (for migration issues) and the XR template (for the export scenario). They both appear to work as expected.

@YuriSizov YuriSizov marked this pull request as ready for review November 22, 2023 18:08
@akien-mga
Copy link
Member

I tested and confirm this fixes #84732.

The toaster message works better now, but on the GDQuest TPS demo, I notice this:
image

image

It's still spamming the individual message for each outdated resource opened via the main scene, which is quite a lot of stuff in that project. The warning about running the surface upgrade tool is repeated each time, and both are mixed in the toaster (though that's a DEV_ENABLED build, I think the file-specific warnings might not be shown in the toaster in non-dev builds).

It's probably still good enough at this stage. As we known it's difficult to establish when the editor is done loading the open scenes to delay showing a single instance of that warning so that it appears at the bottom of the Output dock.
And making it WARN_PRINT_ONCE would risk having it drowned in a sea of other warnings.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Nov 22, 2023

I believe this happens already if you cancel the immediate confirmation dialog (the only difference is that we may suppress it on the initial load, but if you close and reopen it — it should still display the same amount of warnings as you currently see in this PR).

So I would say this is the expected UX for users who currently choose to keep their meshes. We may want to tune this somehow in a follow-up release, but let's see what the feedback is.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested, works as intended.

@akien-mga akien-mga merged commit ee14dc6 into godotengine:master Nov 22, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! Amazing work 🎉

@Chubercik
Copy link
Contributor

What a great branch name 😅

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