Skip to content

Add shape fill overlays when closing a path (Pen tool) or filling it (Fill tool) #2521

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Apr 24, 2025
Merged

Add shape fill overlays when closing a path (Pen tool) or filling it (Fill tool) #2521

merged 28 commits into from
Apr 24, 2025

Conversation

c-mateo
Copy link
Contributor

@c-mateo c-mateo commented Apr 6, 2025 •
edited by Keavon
Loading

Now the pen tool shows a path being closed as requested in #2390 but doesn't work well with vector meshes. The fill tool shows the region to be filled by drawing a filled overlay too. Few new methods were added to the OverlayContext for drawing paths and filling areas.

Closes #2390.

@c-mateo c-mateo mentioned this pull request Apr 6, 2025
Closed
@c-mateo
Copy link
Contributor Author

c-mateo commented Apr 6, 2025

Thank you. The failed build made me notice I forgot to upload one file. Apologies.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 6, 2025

📦 Build Complete for 6e39bc2
https://3d0903e9.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Apr 6, 2025

pen_tool.rs requires formatting

@Keavon Keavon force-pushed the master branch 3 times, most recently from aa7ff13 to e11b57a Compare April 6, 2025 11:41
@c-mateo c-mateo marked this pull request as draft April 7, 2025 03:22
@c-mateo c-mateo marked this pull request as ready for review April 9, 2025 20:03
@Keavon
Copy link
Member

Keavon commented Apr 14, 2025

Please mark your fork as writable by maintainers of this org, since I can't push edits or even resolve conflicts in your PR. I can do a proper code review after that point. Mark this as ready for review once that's ready, thanks.

@Keavon Keavon marked this pull request as draft April 14, 2025 06:31
@Keavon Keavon marked this pull request as ready for review April 14, 2025 11:06
@Keavon
Copy link
Member

Keavon commented Apr 14, 2025

Can you please try a version of the Fill tool where we use the stripes, with a fully opaque version of the fill color, drawn over what's hovered instead of the semitransparent solid version?

@c-mateo
Copy link
Contributor Author

c-mateo commented Apr 14, 2025

Like this but also filled with the primary color?

image

@Keavon
Copy link
Member

Keavon commented Apr 14, 2025

Yes, but with (roughly 2x) denser stripes.

@Keavon
Copy link
Member

Keavon commented Apr 15, 2025 •
edited
Loading

It looks like you've implemented it with a fully opaque fill color + fully opaque blue stripes. My request was just for fully opaque fill color stripes.

Separately, it looks like you changed the Pen tool shape-closing opacity, can you please set that back to how I had tuned it?

@Keavon Keavon marked this pull request as draft April 15, 2025 05:09
@github-actions GitHub Actions
Copy link

📦 Build Complete for b950441
https://39c932e8.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Apr 18, 2025

The shrinking lines bug is still there and pretty easy to replicate using the instructions I gave above.

@Keavon Keavon marked this pull request as draft April 18, 2025 00:11
@Keavon
Copy link
Member

Keavon commented Apr 18, 2025

Looks like it happens only in Firefox.

@c-mateo
Copy link
Contributor Author

c-mateo commented Apr 18, 2025

I just downloaded Firefox 137.0.2 and don't happens to me either. Might it have anything to do with resolution, user's software or hardware?

@seam0s-dev
Copy link

seam0s-dev commented Apr 18, 2025 •
edited
Loading

I did not get to replicate the same error, instead I got this behavior on Firefox 132.0, Fedora Linux 40 Oops, I tested on the wrong commit (https://3d0903e9.graphite.pages.dev/). Ignore these videos :

2025-04-18.10-43-46.mp4
2025-04-18.10-46-49.mp4

For the correct commit, I got the same error on Firefox 132.0, Fedora Linux 40:

2025-04-18.11-19-51.mp4
2025-04-18.11-21-25.mp4

@Keavon
Copy link
Member

Keavon commented Apr 18, 2025

Even weirder! Thanks for testing.

@Keavon
Copy link
Member

Keavon commented Apr 21, 2025

!build

@github-actions GitHub Actions
Copy link

📦 Build Complete for 7354586
https://38b32178.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Apr 21, 2025

!build

@github-actions GitHub Actions
Copy link

📦 Build Complete for 804acfd
https://c10ac5cc.graphite.pages.dev

@c-mateo c-mateo marked this pull request as ready for review April 22, 2025 01:04
@Keavon
Copy link
Member

Keavon commented Apr 23, 2025

!build

@Keavon Keavon changed the title Make the Pen tool show a path being closed by drawing a filled overlay when hovering the endpoint Add shape fill overlays when closing a path (Pen tool) or filling it (Fill tool) Apr 23, 2025
@github-actions GitHub Actions
Copy link

📦 Build Complete for 2e79302
https://210fbf90.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Apr 23, 2025

!build

@github-actions GitHub Actions
Copy link

📦 Build Complete for 3e7be59
https://250e978a.graphite.pages.dev

@Keavon Keavon merged commit 3d37ef7 into GraphiteEditor:master Apr 24, 2025
4 checks passed
@c-mateo c-mateo deleted the 2390-show-path-being-closed branch April 24, 2025 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keavon Keavon Keavon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the Pen tool show a path being closed by drawing a filled overlay when hovering the endpoint
3 participants
@c-mateo @Keavon @seam0s-dev