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 canvas sizing mode, fix preview A/R #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rock3r
Copy link
Contributor

@rock3r rock3r commented Mar 6, 2025

This commit changes the canvas sizing mode; it is now going to either fill available space, or have a fixed size, on both directions. This makes it behave more intuitively, and allows us to properly respect the canvas aspect ratio in the preview. This is reflected in the UI.

image

That change also includes a bunch of cleanup (including addressing Lint complaints) of the project code.

It also includes a change to how points are drawn in the gradient, to avoid the points being squashed for non-square canvases.

Lastly, it adds an explicit output image size in the export scale selector, allowing a better understanding of what each option means.

demo.mp4

@rock3r rock3r force-pushed the fix-canvas-aspect-ratio branch from 7ca72e7 to 83f5968 Compare March 6, 2025 15:39
@c5inco c5inco self-requested a review March 7, 2025 04:11
Copy link
Owner

@c5inco c5inco left a comment

Choose a reason for hiding this comment

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

Left a couple comments. Overall, very welcome changes but need to fix the sizing issue mentioned.

}
)
.thenIf(canvasSizeMode == DimensionMode.Fixed) {
aspectRatio(canvasAspectRatio)
Copy link
Owner

Choose a reason for hiding this comment

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

While this maintains aspect ratio, doesn't behave as expected when setting the size to be smaller than the available space. It also seems to break the double click action to hide/show the points.

It works as expected if setting the size Modifier directly (with the canvasWidth and canvasHeight respectively).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't behave as expected when setting the size to be smaller than the available space

It works as I expected it to :D That is, take up as much space as possible for the preview. This solves issues with having to implement zoom levels, too, for cases where the canvas is too small to be worked on. If you set the size directly, then you need to implement panning and zooming for when the canvas is too small or large, I reckon, and I didn't want to have to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot:

It also seems to break the double click action to hide/show the points

Works for me — at least when double clicking on the preview canvas. Not elsewhere (but that was already the case before, if you made the canvas non-fill, and small enough to fit in the viewport)

Screen.Recording.2025-03-07.at.16.37.59.mov

Copy link
Owner

Choose a reason for hiding this comment

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

This solves issues with having to implement zoom levels, too, for cases where the canvas is too small to be worked on. If you set the size directly, then you need to implement panning and zooming for when the canvas is too small or large, I reckon, and I didn't want to have to do it

Gotcha. I would prefer to reflect the size at 100% even if it's too small to work with in some situations. Currently I find it confusing when I set it to a smaller size than the available space and visually feels to be roughly the same size. Maybe if we had a label that expressed what zoom level it is at, it would be clearer.

Screen.Recording.2025-03-07.at.21.02.56.mov

To that end, I think we will need zoom levels at some point so I would prefer to fix this sizing issue then.

Copy link
Owner

Choose a reason for hiding this comment

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

Forgot:

It also seems to break the double click action to hide/show the points

Works for me — at least when double clicking on the preview canvas. Not elsewhere (but that was already the case before, if you made the canvas non-fill, and small enough to fit in the viewport)

Screen.Recording.2025-03-07.at.16.37.59.mov

Yeah it seems to be fine now. Not sure what was happening when I saw it not working.

This commit changes the canvas sizing mode; it is now going to either
fill available space, or have a fixed size, on both directions. This
makes it behave more intuitively, and allows us to properly respect
the canvas aspect ratio in the preview. This is reflected in the UI.

That change also includes a bunch of cleanup (including addressing
Lint complaints) of the project code.

It also includes a change to how points are drawn in the gradient, to
avoid the points being squashed for non-square canvases.

Lastly, it adds an explicit output image size in the export scale
selector, allowing a better understanding of what each option means.
@rock3r rock3r force-pushed the fix-canvas-aspect-ratio branch from 83f5968 to f002f16 Compare March 7, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants