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

Add a button in the export dialog to fix missing texture formats #78457

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 20, 2023

This PR improves the error message about texture formats by adding a button to fix it. I was inspired by a similar fix button in VRChat and realized the same idea would work perfectly here. This will greatly help with #78395.

Screenshot 2023-06-19 at 9 09 50 PM

When the button is pressed, the project setting is enabled, the settings are saved, the editor file system is re-scanned (which reimports all textures as needed), and the export dialog is refreshed.

@YuriSizov
Copy link
Contributor

From the discussion we had in the production meeting today I can say that this is a good improvement, but there may be some suggestions on how to adjust this new UI. So to give it a bit time to polish I'm putting it onto 4.2. #78456 should already help a lot in 4.1.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 20, 2023
@akien-mga akien-mga requested review from clayjohn and YuriSizov and removed request for a team August 7, 2023 10:25
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Haven't tested, just the code and style review. Overall the editor bits look fine.

Design-wise, I think we use LinkButton (or meta tags if in RTL) for such actions, though. A normal button stands out more and is easier to understand, but it also kind of looks out of place. Not a blocker though.

editor/export/editor_export.h Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
editor/export/project_export.cpp Outdated Show resolved Hide resolved
@aaronfranke aaronfranke force-pushed the tex-format-fix-button branch 3 times, most recently from b01763f to 5ad3883 Compare August 9, 2023 02:30
@aaronfranke
Copy link
Member Author

Design-wise, I think we use LinkButton (or meta tags if in RTL) for such actions, though. A normal button stands out more and is easier to understand, but it also kind of looks out of place. Not a blocker though.

Usually you would expect a LinkButton to, well, be a link to something, like opening a menu or web page. This button doesn't link to anything, it performs an action. A regular button makes more sense to me here.

@aaronfranke aaronfranke force-pushed the tex-format-fix-button branch 3 times, most recently from 71ae7a5 to 82601cc Compare August 9, 2023 16:22
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.

The overall feature looks good to me.

Also not convinced by the looks of this "Fix Import" button which looks a bit out of place in an error message IMO, but we can iterate on this further later.

@aaronfranke
Copy link
Member Author

Updated the PR to use a LinkButton (and re-tested).

Screenshot 2023-08-17 at 2 56 21 AM

@akien-mga
Copy link
Member

I like the look if it. I somewhat agree with your previous statement:

Usually you would expect a LinkButton to, well, be a link to something, like opening a menu or web page. This button doesn't link to anything, it performs an action.

But somehow it still looks nicer like this :P

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Changes look good from my perspective!

@akien-mga akien-mga merged commit 229af8e into godotengine:master Aug 17, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the tex-format-fix-button branch August 17, 2023 14:58

void ProjectExportTextureFormatError::_notification(int p_what) {
switch (p_what) {
case NOTIFICATION_ENTER_TREE:
Copy link
Contributor

Choose a reason for hiding this comment

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

This was excessive btw, NOTIFICATION_THEME_CHANGED already fires when the node enters the tree.

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