Arc Center Point

[nicholalexander]

following up on 2nd half of #996

Same implementation, but notice the commented out test for centered arcs. This doesn't work, and I'm not sure what the expected behavior of it should be. It is also a problem on stars. I wasn't sure if I should file this and then open a bug or no?

Also, I tried looking at where centering gets applied but have not yet been successful in understanding how those dimensions actually get created - except within the SWT code?

[jasonrclark]

Ah, that's a good catch on centering. The centering gets applied down at the Shoes::Dimensions level (which is the class that the various elements use to track their actual dimensions).

It is likely that you'll just need a conditional around whether you're centered or not in your accessors--that's effectively what the core dimensions do.

If you want to address the centering with the Arc code here, that'd be fine. In either case, an issue documenting the problem for Star (and if you don't want to do it here, Arc) would be appreciated!

[jasonrclark]

Another thought as well... if the implementation of this center point is precisely the same, I wonder if it could be extracted to a Shoes::Common module, or implemented on the Shoes::ArtElement? Then when we have to do funky things about centering for instance, it'd be all in one place to fix (centralized... hahaha).

[nicholalexander]

@jasonrclark - thanks! I fixed the problem with arc here and opened another issue for the star. If this fix is right, I'll go back and apply to star. Tests passing locally.

Also, love the idea of centralizing all centerable things - let me get this and the star working and then i can open a new PR to refactor the methods into the common module - does that sound reasonable?

[jasonrclark]

Does the read side of this accessor need to factor in whether we're centered or not as well?

[nicholalexander]

I'm not sure. I think this logic is behaving as we would want. Here's a sample pair of shoes I was running around in and the center_point accessor match the x and y cursor - which I think is what it should be doing?

Shoes.app width: 420, height: 420, resizable: false do
  stroke rgb(0, 0.6, 0.9)
  cap :curve
  nofill
  strokewidth 10
  
  @arc = arc 210, 300,
             200, 200,
             -(Shoes::HALF_PI / 3),  -(Shoes::HALF_PI / 3) + Shoes::TWO_PI - Shoes::HALF_PI,
             center: true

  motion do |x, y|
    @arc.center_point = Shoes::Point.new(x,y)
    puts "cursor position: x: #{x} y: #{y}\r"
    puts "arc center_point: x: #{@arc.center_point.x} y: #{@arc.center_point.y}\r"
  end
end

I think this is because the style[:center] = true is correctly manipulating the shape, but it was only when i was directly setting the top and left I had to account for the center style.

Also, in looking at the test for the centered arc, it seems like that would catch it if this behavior changed in the future.

Sorry, does any of this seem right? Also, do you think I should include the above sample in this PR?

[jasonrclark]

So here's a sample app that hopefully clarifies where I think there's an issue:

Shoes.app do
  @s1 = star 100, 100, 5, 90, 50, fill: orange
  @s2 = star 100, 100, 5, 90, 50, fill: pink, center: true

  stack do
    @info = para ""
    para @s1.center_point
    para @s2.center_point
  end

  click do |*args|
    @info.text = args.inspect
  end
end

In this sample, these two stars get set with the same location x, y values at creation, but one's center: true and the other isn't. These obviously draw in different spots on the screen.

The para's at the top report the center_point, and with the current logic they both report the same value even though they should be different. Does that make sense?

Would love a nice centered arc example. samples/simple_arc.rb is already taken, but feel free to add one!

[nicholalexander]

Cool, I totally see that. But with arc's I think it seems to work. I added to your program and I think they are correctly reporting their center points. Since we have #1531 opened, and since I wasn't sure about refactoring the center_point into the common module, I figured I'd address the stars under a separate PR meant to address 1532. If you think that still makes sense.

Shoes.app do
  @s1 = star 100, 100, 5, 90, 50, fill: orange
  @s2 = star 100, 100, 5, 90, 50, fill: pink, center: true

  @a1 = arc 200, 200, 200, 200, 3.141592654, 0, fill: blue
  @a2 = arc 100, 300, 200, 200, 3.141592654, 0, fill: red, center: true
  
  stack do
    @info = para ""
    para @s1.center_point
    para @s2.center_point
    para @a1.center_point
    para @a2.center_point
  end

  click do |*args|
    @info.text = args.inspect
  end
end

Also, I added a sample program under simple_rainbow.rb - a nice happy rainbow! I had a question about the documentation for shoes4 and I'm not sure where the right place to ask that is?

Thanks for all your feedback! I hope I'm not actually creating more work for you! Let me know what you think about the arc's working and if I can fix the Star accessor in that separate PR.

[jasonrclark]

Oh, that's super interesting! I thought the center point vs left/top thing was handled by underlying code, so I'm surprised that they're different between the two.

We can totally carry on and address the star issues with #1531 since arcs aren't impacted.

And no worries at all about creating work. I love helping folks get involved and get to know the code, and you're making good changes that it's fun to follow along with. Shoes on!

[jasonrclark]

Had a question on the current implementation, but the plan to 1) get this merged, 2) apply it to star 3) refactor out to common module sounds 👌 to me.

[jasonrclark]

So with my question about center_point out of the way, is this ready to roll? Love the new sample BTW 🌈

[nicholalexander]

Great! I really liked making the rainbow so I'm glad its a good sample! And yes, ready to roll on this PR, thanks!

Also just as a follow up on the documentation question, is there an open active issue or pr for that or should i start a new one?

[jasonrclark]

Cool. Will get that merged.

I seem to have missed the doc question. If you’re interested in general documentation we do have #620 our there, along with a few other specific tickets for particular things that need docs.