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

Increase vertical size of CurveEdit when Inspector widens #77625

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented May 29, 2023

This should allow users to edit points in a less constrained space, which feels like a UX improvement.

image

That said, per @YuriSizov, changing minimum size according to current size might be a hack that causes issues in certain situations, so there's a good chance this shouldn't be accepted :)

Also: known issue that if you open the curve editor then maximize the entire editor window, this does not appear to take effect until you actually resize the inspector tab. Possibly a symptom of the above issue?

@YuriSizov YuriSizov added this to the 4.x milestone May 29, 2023
@YuriSizov YuriSizov requested review from YuriSizov and a team June 12, 2023 13:45
@YuriSizov
Copy link
Contributor

@anvilfolk Would you mind rebasing this?

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 3, 2023
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.

Once rebased, this can be merged.

This is indeed a problematic approach that can lead to unstable size resolution. But it works so far. If users experience issues we can always revert the change.

Also we this is okay as long as we don't expose this widget. If we do expose it, some other containers or situations can lead to our favorite infinite sizing bug. So this is something to consider, as there are a lot of requests for us to expose this.

This should allow users to edit points in a less constrained space,
which feels like a UX improvement.

That said, changing minimum size according to current size might be a
hack that causes issues in certain situations.
@anvilfolk
Copy link
Contributor Author

anvilfolk commented Aug 3, 2023

Rebased!

I am worried about the issues you have mentioned all along, since I don't have a ton of experience with editor/inspector. Let's keep an eye out for reports of slowdowns/lag when resizing the inspector with the curve editor open since that could come from this infinite/many iteration size update issue.

If problems end up being significant, we can revert this and give the editor a few settings for vertical size (e.g. small, medium, large) which can be stored in the curve's metadata. The UX wouldn't be as good, but would be more likely to avoid these issues!

I am glad we are giving this a try since it does make a difference when working with the curve editor :)

@YuriSizov YuriSizov merged commit 54c0d0f into godotengine:master Aug 3, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@anvilfolk anvilfolk deleted the itgrowwws branch August 3, 2023 20:52
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