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

Fix for Canvas.rows() output being 1 row/col too large when Canvas::new() is passed particular values #15

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

Conversation

TristanTurcotte
Copy link

Fixes #14, where output string can be 1 row and/or col too large resulting in additional white space when given width is divisible by 2 and/or height is divisible by 4.

The only time this issue occurs is when Canvas::new() is called and the given width/height are divisible by the 'braille-pixel' ratio, where width is divisible by 2 and height by 4. For example if a width of 12 is given, the Canvas struct's width would be 6 and when rows() is called, the maxrow variable would be off by one and cause an additional empty column to appear in the output string. Another example, if a height of 4 is given, the Canvas' height would be 1 and when rows() is called, the maxcol variable would be set to 1, allowing the loop to run an additional iteration and inserting an empty row in the output string.

Prior to this fix, the following code:

let mut canvas = Canvas::new(2, 4);
for y in 0..4 {
    for x in 0..2 {
        canvas.set(x, y);
    }
}
dbg!(canvas.frame());

Would output the following:

[examples\test_canvas_properties.rs:14] canvas.frame() = "⣿ \n  "

And with the fix the output is instead:

[examples\test_canvas_properties.rs:14] canvas.frame() = "⣿"

Let me know if there's anything else I could do or if there's an issue with this pull request. Thank you!

…ixel width/height got to the character boundary.
@TristanTurcotte
Copy link
Author

An alternative to this quick fix would be to instead dynamically update the width and height member fields of Canvas whenever it is modified, with for example a check_bounds(u16,u16):

fn check_bounds(&mut self, row: u16, col: u16) {
   if self.width < row + 1 {
      self.width = row + 1;
   }
   if self.height < col + 1 {
      self.height = col + 1;
   }
}

And to also properly calculate the Canvas width and height in the constructor:

pub fn new(width: u32, height: u32) -> Canvas {
   let width_adjustment: u16 = if width != 0 && width % 2 == 0 { 0 } else { 1 };
   let height_adjustment: u16 = if height != 0 && height % 4 == 0 { 0 } else { 1 };
   Canvas {
      chars: FnvHashMap::default(),
      width: (width / 2) as u16 + width_adjustment,
      height: (height / 4) as u16 + height_adjustment,
   }
}

With these changes, the bounds checking for loop in Canvas.rows() can be removed and the loop ranges become exclusive rather than inclusive:

pub fn rows(&self) -> Vec<String> {
   let maxrow = self.width;
   let maxcol = self.height;

   let mut result = Vec::with_capacity(maxcol as usize + 1);
   for y in 0..maxcol {
      let mut row = String::with_capacity(maxrow as usize + 1);
      for x in 0..maxrow {
   // -- snip --
}

I have implemented these changes in a dev branch on my fork, /TristanTurcotte/drawille-rs/tree/dev

Let me know and I can push those changes into this Pull Request. I could also include Canvas.get_width() and Canvas.get_height() additions if you wish.

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.

Size of initialized canvas
1 participant