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

Improve blend tree contrast/margins #94721

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

ckaiser
Copy link
Contributor

@ckaiser ckaiser commented Jul 25, 2024

Improved light theme contrast and readability by adding some margins and adjusting some theme stuff that I think might be leftovers from previous PRs that made adjustments to this.

Fixed graph nodes using get_size() instead of get_minimum_size() when they weren't resizable which could result in the nodes retaining their original size after changes. I could not find what exactly caused this regression since 4.2 but this seems like it should be the intended behavior anyhow, if the user can't resize the graph node, it should just adjust to its contents automatically.

4.2 4.3 This PR
before_4_2.mp4
before_4_3.mp4
after.mp4

@ckaiser ckaiser requested review from a team as code owners July 25, 2024 06:53
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

In which cases does the regression appear? Could you upload an minimal reproduction project?

@ckaiser
Copy link
Contributor Author

ckaiser commented Jul 27, 2024

In which cases does the regression appear? Could you upload an minimal reproduction project?

I'll try to make an MRP this weekend to check under what circumstances it happens, the only one that was clear was when opening and closing the resource picker in StateMachine nodes in blend trees.

I'm not sure if it's something specific to this particular node (EditorResourceProperty) interacting with the graph node so I'll do some more digging

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 31, 2024
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jul 31, 2024
@ckaiser ckaiser force-pushed the animation-blend-tree-tweaks branch from 5525100 to 9bcbcc8 Compare August 1, 2024 07:37
@ckaiser
Copy link
Contributor Author

ckaiser commented Aug 1, 2024

I've done some testing, and I don't think the issue comes from the graph node itself, but rather something else that's not allowing the resize to trigger and make the node recalculate its size.

For this particular specific case I've removed the minimum size change and added a fix that restores the original behavior in the blend trees graph, since I haven't seen anything else affected by this I think this is the path with the least friction, if slightly hacky

@Geometror Geometror changed the title Fix graph node sizing regression, improve blend tree contrast/margins Improve blend tree contrast/margins Nov 19, 2024
@Geometror
Copy link
Member

Geometror commented Nov 19, 2024

I updated the PR title and description to reflect that.
But do you still have the minimum size patch around?

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

I rebased and tested this PR locally - looking good!
Currently, the embedded inspector of nodes seems to be broken (won't even show up with error Parameter "parent inspector" is null.), but that's unrelated to this PR (verified).

@ckaiser ckaiser force-pushed the animation-blend-tree-tweaks branch from 9bcbcc8 to 5559075 Compare November 22, 2024 19:17
@ckaiser ckaiser requested a review from a team as a code owner November 22, 2024 19:17
@ckaiser
Copy link
Contributor Author

ckaiser commented Nov 22, 2024

Rebased, and I can confirm the embedded inspector is broken now, unrelated to the PR

@passivestar
Copy link
Contributor

This PR fixes a problem that I recently noticed in my theme where progress bars with expanded margins wouldn't even fit within the nodes

g

I had to remove expanded margins because of that, making all of the progress bars in the editor smaller. When this PR is merged I can make progress bars bigger again

@akien-mga akien-mga merged commit 6d4bb8f into godotengine:master Nov 29, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release enhancement regression topic:editor topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants