Add `Grid` helper for grid-based layout by afternoon2 · Pull Request #43 · abey79/vsvg

afternoon2 · 2023-10-08T09:09:56Z

No description provided.

abey79 · 2023-10-08T15:42:29Z

I honestly feel that both the implementation and the user-facing API would be much cleaner with a callback approach. For starters, I suggest leaving aside the question of configurability and use a builder pattern for the Grid.

I suggest implementing for that kind of API:

fn update(&mut self, sketch: &mut Sketch, _ctx: &mut Context) -> anyhow::Result<()> {
    Grid::from_total_size(400.0, 300.0)   // alt: .from_cell_size(dx, dy)?
        .rows(12)
        .columns(15)
        .spacing(15.) // alternative: .vertical_spacing / .horizontal_spacing
        // other parameters?
        .transform(true)   // this has to be optional for perf reason
        //.scale(true) // that would be neat, to map the cell area to x = 0..1, y = 0..1
        .build(sketch, |sketch, cell| {
            sketch.rect(0.0, 0.0, cell.width, cell.height);
        });
}

Then Cell is just a very stupid struct that stores some basic stuff (i, j, width, height, dx, dy, etc.

abey79

I love this feature already, but let's go the extra mile and give it the best API :)

abey79 · 2023-10-08T15:44:13Z

+    /// Canvas position
+    pub canvas_position: Point,
+    /// User-defined data
+    pub data: Option<T>,


The implementation will be much cleaner without having to deal with any user data

good idea with the builder pattern. Idk why, but I used it when implementing the very same grid in JS, but forgot it when doing same in Rust.

abey79 · 2023-10-08T15:45:30Z

+pub enum GridSize {
+    /// Set cell size, grid's size will be computed
+    CellBased([f64; 2]),
+    /// Set fixed grid size
+    GridBased([f64; 2]),
+}


Ideally we can make this sort of thing an implementation detail (i.e. not pub) and expose it through nice builder pattern.

abey79 · 2023-10-08T15:45:50Z

+/// );
+/// grid.init(None);
+/// ```
+pub struct Grid<'a, T> {


Grid is a much better name than GridHelper or whatever else 👍🏻

abey79 · 2023-10-08T15:46:15Z

+    /// Creates new instance of the grid with empty data vector
+    #[allow(clippy::too_many_arguments)]
+    pub fn new(
+        sketch: &'a mut Sketch,


This sort of annoyance disappears with my proposal.

abey79 · 2023-10-08T15:47:18Z

+    pub fn at(&mut self, column: usize, row: usize) -> Option<&mut GridCell<T>> {
+        self.data
+            .iter_mut()
+            .find(|cell| cell.column == column && cell.row == row)
+    }
+
+    /// Resets grid cell's data at specific column and row if applicable
+    pub fn reset_at(&mut self, column: usize, row: usize) {
+        let cell = self.at(column, row);
+
+        if let Some(cell) = cell {
+            cell.reset_data();
+        }
+    }
+
+    /// Resets all grid cell's data
+    pub fn reset(&mut self) {
+        self.data.iter_mut().for_each(GridCell::reset_data);
+    }


Being a nice "container of user data" is tough and, again, best avoided altogether.

afternoon2 · 2023-10-10T07:39:48Z

@abey79 I made a refactor with builder pattern, but without transform and scale methods. Wdyt about putting them into issues with feature label to be implemented in separate PRs?

abey79

This looks good. Here are bunch of comments to make it even better. The main thing is that I think we should adhere rather strictly to the builder pattern in this case. This makes for better performance and more obvious/standard API.

abey79 · 2023-10-13T16:12:51Z

+                let c = cell.clone();
+                sketch.color(Color::RED);
+                sketch.add_path(c);


I don't like this clone, this is unergonomic. Let's have IntoBezPathTolerance implemented for &GridCell, so we don't need it.

abey79 · 2023-10-13T16:17:02Z

+        if let Some(marked_cell) = grid.at(self.marked_cell_col, self.marked_cell_row) {
+            sketch.color(Color::GREEN);
+            sketch.add_path(marked_cell.clone());
+        }


It's weird to me that there are two ways to access a GridCell: via the closure, and later on because they are somehow stored. If GridCell contains row/col information, it's very easy to add this to the closure:

sketch.color(if cell.row == self.marked_cell_row && cell.col == self.marked_cell_col { Color::GREEN } else { Color::RED });

I would remove that GridCell storage all together. If the user really needs to store those cell, it can be done via the closure.

abey79 · 2023-10-13T16:17:30Z

@@ -0,0 +1,217 @@
+//!


No need for docstring here.

abey79 · 2023-10-13T16:19:05Z

+}
+
+/// Stores basic grid's cell data, like column, row and canvas position
+#[derive(Clone)]


Let's add the usual suspects (Debug, PartialEq etc.) we don't know what the user might need to do with these.

abey79 · 2023-10-13T16:32:25Z

+    column: usize,
+    row: usize,
+    position: Point,
+    size: [f64; 2],


These fields need to be pub and well documented (in particular, what position wrt to the current transform, etc.). We don't know what the user might want to do with those.

abey79 · 2023-10-13T16:48:57Z

+    /// Aligns grid cells to specific dimensions
+    pub fn align_to(&mut self, rect_dimensions: [f64; 2]) {
+        let [width, height] = self.module_size();
+        let diff_x = rect_dimensions[0] / width;
+        let diff_y = rect_dimensions[1] / height;
+
+        self.data.iter_mut().for_each(|cell| {
+            cell.position = Point::new(cell.position.x() + diff_x, cell.position.y() + diff_y);
+        });
+    }


I'm not sure I understand what this does, but it should follow the builder pattern.

It was for aligning the grid to a given artboard rectangle, but since we're able to align things to the sketch with existing sketch.center method I think we can delete it

abey79 · 2023-10-13T16:49:59Z

+
+    /// Overrides grid's current number of rows.
+    /// By default, grid instance will have 4 rows.
+    pub fn rows(&mut self, value: usize) -> &mut Self {


In this case, we should adhere strictly to the builder pattern, so take self and return Self. Likewise for all other functions.

honestly, I didn't know once could use fn rows(self, value: usize) -> Self with just self in return instead of creating everything from scratch there, like: Self { /* properties go here */ } 😅

abey79 · 2023-10-13T16:54:39Z

+    /// Returns width of the grid
+    #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)]
+    pub fn width(&self) -> f64 {
+        let columns = self.dimensions[0] as f64;
+        columns * self.module_size()[0] + columns * self.gutter[0]
+    }
+
+    /// Returns height of the grid
+    #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)]
+    pub fn height(&self) -> f64 {
+        let rows = self.dimensions[0] as f64;
+        rows * self.module_size()[1] + rows * self.gutter[1]
+    }


I've got mixed feelings about these. They could be useful, but they to fit the builder pattern well. That info could be part of the GridCell (to be made available to the closure).

good point. I just treated these methods as "getters"

abey79 · 2023-10-13T16:56:08Z

@@ -0,0 +1,70 @@
+use whiskers::{grid::Grid, prelude::*};


Grid should be in the prelude so that use here is not needed.

abey79 · 2023-10-13T16:57:24Z


 mod context;

+pub mod grid;


Let's remove pub here and pull everything in the top-level namespace with pub use grid::{/*:..*/}

abey79

Looks great now! A nice, clean mvp feature.

abey79 · 2023-10-15T14:08:11Z

+        grid.columns(self.columns)
+            .rows(self.rows)
+            .spacing([self.gutter_width, self.gutter_height])
+            .build(sketch, |sketch, cell| {
+                sketch.color(
+                    if cell.row == self.marked_cell_row && cell.column == self.marked_cell_col {
+                        Color::GREEN
+                    } else {
+                        Color::RED
+                    },
+                );
+                sketch.add_path(cell);
+            });


This looks very nice now 👍🏻

afternoon2 added 5 commits October 8, 2023 10:06

add grid module to whiskers

8c59ab6

move grid drawing to struct method

62dc16b

add custom data fill function to the example

3b158b0

fix lifetime

cb8d57f

Update grid.rs

7015afc

abey79 reviewed Oct 8, 2023

View reviewed changes

afternoon2 added 6 commits October 9, 2023 21:06

wip new grid

a3d4d3f

add at() and align_to()

aa89e6a

add clippy annotations

affc1ef

add callback closure to build method

ae0907f

add build method, refactor code organization

e82183a

doc update, remove pub use geo_impl

5d1a1db

afternoon2 marked this pull request as ready for review October 10, 2023 07:38

afternoon2 requested a review from abey79 October 10, 2023 07:38

use impl instead of generic in build method

124869f

abey79 requested changes Oct 13, 2023

View reviewed changes

afternoon2 added 2 commits October 14, 2023 12:51

PR changes p. 1

fc837c3

PR changes p. 2

9cac8a9

afternoon2 requested a review from abey79 October 14, 2023 11:00

abey79 added 2 commits October 15, 2023 16:12

Merge branch 'master' into whiskers-grid

1c08536

Misc clean ups

ae3a6d8

abey79 approved these changes Oct 15, 2023

View reviewed changes

abey79 changed the title ~~Grid module proposal for whiskers~~ Add Grid helper for grid-based layout Oct 15, 2023

abey79 merged commit 1ef1c74 into abey79:master Oct 15, 2023

Conversation

afternoon2 commented Oct 8, 2023

Uh oh!

abey79 commented Oct 8, 2023

Uh oh!

abey79 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afternoon2 commented Oct 10, 2023

Uh oh!

abey79 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abey79 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants