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

Dot grid #1709

Merged
merged 7 commits into from May 8, 2024
Merged

Dot grid #1709

merged 7 commits into from May 8, 2024

Conversation

adamgerhant
Copy link
Collaborator

@adamgerhant adamgerhant commented Mar 27, 2024

Superseded by #1743. This PR wasn't actually merged into master.


Closes #1704

Adds a dot type grid option. Snapping and spacing is the same as the rectangular grid.

Changes:

  • Adds grid_overlay_dotfunction to draw circles using the overlay context
  • Adds GridType::Dot with similar implementation to GridType::Rectangle
  • UI changes for selecting the Dot option
  • Replaces matches! macro for selecting index to a match statement
  • Adds circle function to utility_types.rs

dots

@adamgerhant
Copy link
Collaborator Author

adamgerhant commented Mar 27, 2024

Closes #1703

Changes:

  • Adds grid_color field to GridSnapping
  • Color selector UI
  • Adds support for prefixed hexed codes

Note: blocked by #1710 since initial color is not correct.

color_select

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

Thanks for your effort so far.

node-graph/gcore/src/raster/color.rs Outdated Show resolved Hide resolved
node-graph/gcore/src/raster/color.rs Outdated Show resolved Hide resolved
@adamgerhant
Copy link
Collaborator Author

adamgerhant commented Mar 28, 2024

Thank you for the input. I will be taking a break from this PR for the next few days while I work on my GSoC proposal. Once that's done I'll continue working on it

@adamgerhant
Copy link
Collaborator Author

I believe all changes should have been made. I removed the Dot grid type, and instead added a Boolean field that tracks whether the grid should be displayed with dots. This also makes it easy to add a dot grid for the Isometric type, if there are plans for that. Just remove the condition for displaying the checkbox, create the display function, and use it accordingly.

I removed the prefixed color methods and used concatenation and remove_prefix accordingly. Currently this PR is blocked by the Color bug, since it starts at #999999 instead of #cccccc. An easy fix could be to replace the hex constant with a Color.

@Keavon
Copy link
Member

Keavon commented Apr 4, 2024

!build

Copy link

github-actions bot commented Apr 4, 2024

📦 Build Complete for ad78699
https://725c1a4f.graphite.pages.dev

@adamgerhant
Copy link
Collaborator Author

adamgerhant commented Apr 4, 2024

Performance is very poor for the dot grid. I suppose this is because x_count+y_count lines need to drawn for the rectangle grid, but x_count*y_count circles need to be drawn for the dot grid. Additionally, the .arc call to draw circles is pretty slow. I think the best way to fix this would be to replace the lines with dashed lines, but then the dots will be rectangular. Another method could be to only redraw the circles when the viewport is resized or the color is changed.

@Keavon
Copy link
Member

Keavon commented Apr 4, 2024

Square 1px pixel grid aligned (no antialiasing) dots would already be the desired appearance. Just be aware that a dotted line might drift if the ratio is a non-integer fraction between the document space and viewport space, so some cleverness might be required to enable the avoidance of antialiasing by staying perfectly grid aligned. Maybe several lines per row/column with different screenspace pixel spacing intervals as the error accumulates between the non-integer fraction ratio and the ratio you need to maintain screenspace grid alignment.

@Keavon
Copy link
Member

Keavon commented Apr 18, 2024

I'm putting this back to draft status till you have time again to deal with the performance suggestions in my previous comment. But don't focus on this till your other PR is done, since that's the much higher priority feature. Thanks :)

@Keavon Keavon marked this pull request as draft April 18, 2024 07:37
@Keavon Keavon force-pushed the master branch 2 times, most recently from 7bf7f92 to 5f4960d Compare April 25, 2024 01:06
@Keavon
Copy link
Member

Keavon commented May 8, 2024

!build

Copy link

github-actions bot commented May 8, 2024

📦 Build Complete for 40312d6
https://6cf8ab59.graphite.pages.dev

@Keavon Keavon merged commit 40312d6 into GraphiteEditor:master May 8, 2024
2 checks passed
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.

Grid as dots
3 participants