Replace draw_line/triangle with draw_quad - Resolves #315 ...

[Kjarrigan]

... & Add a thickness option to draw_lines

It finally resolves at least the visual bugs in the macros and removes a bit of code-complexity. No fancy lines unfortunately so far but a thickness as first step might be a good direction.

Sort of unmergeable at the moment because the screenshot commits are bundled.

[Kjarrigan]

I thought about removing this attribute completely but couldn't figure out how this would work then:

DrawOpQueue.hpp:142

            manager.set_render_state(op.render_state);
            if (op.vertices_or_block_index >= 0) {
                op.perform(0);
            }
            else {
                // GL code

[jlnr]

I think it could be just int block_index and a value of -1 indicates that it is not a block, but a quad.

[jlnr]

+space around =

[jlnr]

I'm not a fan of adding a parameter at the end :/ But I also don't see any sane way to avoid this without subtly breaking code.

[Kjarrigan]

I would prefer it right before z as well but no idea how to not break code too. I mean in Ruby we could use a named parameter but a list of 8(!) normal and 1 named param would look a bit akward I think. But thickness seems like a good Addition to use draw line especially once someone codes even fancier lines 😛

[jlnr]

Yeah, the 8 parameter thing is another good theme for post-1.0! :/

[jlnr]

+space around -

[jlnr]

Also, you don't need to use Gosu:: inside a Gosu method (I've started to clean this up recently, I used to be undecided about this)

[jlnr]

And you can ignore the call if x1 == x2 && y1 == y2, in which case Gosu::angle cannot return anything useful.

[jlnr]

If this is not needed for the line-drawing code, let's leave it out of this PR

[jlnr]

OK, so I think the best course of action is:

• I'll fix the CI on master
• Merge Ruby 2.5 support
• We merge the screenshot PR
• You rebase this PR, I'll take a quick look again and then we merge it as well
• I finish up my font refactoring stuff (+ screenshot tests)
• Release Gosu 0.14
• Then we can look at the audio class merger?

[Kjarrigan]

Sounds good to me 👍

[Kjarrigan]

Ok rebased and everthing is green - thanks to the Image.similar? 😄 After sleeping a night over it I'm really happy with this solution. Now the way is free for all kinds of tests.

Also changed the vertices_or_block_index to block_index and removed some unnecessary code (like the double negation)

I'm ready for review @jlnr

[jlnr]

(Haven't look at the code yet, on my way to work) - I think we should be careful with similar? because one important property of draw_line is how it handles its first and last pixel. People who want to draw a translucent (alpha=0.5) rectangle for debugging purposes need to understand how draw_line handles this to avoid having four pixels with alpha=0.75 in the four corners of the rectangle. Off the top of my head, I don't even know how draw_line should handle this to make it as easy as possible.

I also found an open browser tab where I hadn't finished typing my comment on the last PR, so here it goes :) I think the assert_similar test helper should always save the image when a test fails (but only then). I don't see why anyone would ever start the tests without being interested in failing image comparisons. Then the only effect of the DEBUG flag would be to print the to_blob diff on Jenkins/Appveyor, has that proven to be useful? Or can we then kick the env variable out again?

(I wonder if Gosu shouldn't generally respect a GOSU_DEBUG or GOSU_VERBOSE env variable even during runtime, to log texture allocations etc...)

[Kjarrigan]

I also found an open browser tab where I hadn't finished typing my comment on the last PR, so here it goes :) I think the assert_similar test helper should always save the image when a test fails (but only then). I don't see why anyone would ever start the tests without being interested in failing image comparisons. Then the only effect of the DEBUG flag would be to print the to_blob diff on Jenkins/Appveyor, has that proven to be useful? Or can we then kick the env variable out again?

Already saw this note on the project tab 😄 The "problem" is that minitest has no built-in do-on-assertion-failed logic and I wasn't sure if adding worth it but I agree - get the image on failure was my first idea as well. The to_blob diff was really helpfull on the CI because there is no way we can download the actual images from there (as far as I can tell). So I copy&pasted the Diff. Removed all lines with a '-' then put it into IRB to convert from Base64 back to Binary and then saved it to file (thats how I found out, that AppVeyor was always off by a Pixel in the triangles). Getting rid of DEBUG is fine by me. Resolved in latest commit

(I wonder if Gosu shouldn't generally respect a GOSU_DEBUG or GOSU_VERBOSE env variable even during runtime, to log texture allocations etc...)

👍

to avoid having four pixels with alpha=0.75 in the four corners of the rectangle. Off the top of my head, I don't even know how draw_line should handle this to make it as easy as possible.

I think thats what happens at the moment and would be at least what I expect - First and last pixel should be part of the line. And if you use them for "Debugging" I guess you could live with a minor color inconsitency but I get the point. As there is at the moment no way to draw an unfilled triangle/rect this might be a thing to consider. So there three options I guess:

• leave it as it is (first and last pixel are part of the Line)
• provide a parameter (yay more parameters...) to change this behaviour (Include Both, Include First, Include Last, Dont Include)
• add an option to draw_triangle/draw_rect to only draw the borders (maybe with thickness? and different colors for border and fill)

[Kjarrigan]

Another thought - at the moment the thickness only makes the line bigger but not longer, where the given coordinates define the middle line and half the thickness is added left and right but maybe it should extend the line by half the thickness at both ends at well? How does drawing programs handle this?

Image 1) for clarification. Blue is how it is at the moment. Red as it maybe should be. Green the given coordinates

Image 2) thats how your rectangle from 4 lines looks at the moment. Added this as test (although more of an reminder at the moment how it works)

[jlnr]

I've opened #440 for the GOSU_DEBUG/GOSU_VERBOSE thingie.

As for the line drawing, I have also thought about these questions and I think I'm opposed to even the single additional thickness parameter. It seems like low-hanging fruit, but there are too many parameters already, and there's still so much that's missing, such as line caps, line joints, and correctly connecting translucent line segments to avoid double-painted areas. Most importantly, any sane line-drawing API should take an array of points, or even better, an arbitrary line/bezier path. The established pattern is to have a Canvas object with appropriate builder methods.

If anyone really needs to draw pretty lines/shapes, they should implement it on top of Gosu.draw_quad or Image.draw. It's fine if draw_line is only useful for quick 1px debugging lines and rectangle frames. I think it was a mistake for Gosu to even offer such a rarely used method as draw_triangle, but I don't want to remove it for the usual backward compatibility reasons (some people do use it).

[jlnr]

On the other hand – if Gosu.draw_line is mostly useful for drawing rectangle frames, then it should at least be perfect for this one use case :) i.e. drawing a line from integer coordinates to integer coordinates should render something that is aligned with physical pixels, not ±0.5px (blurry).

[Kjarrigan]

I think it was a mistake for Gosu to even offer such a rarely used method as draw_triangle, but I don't want to remove it for the usual backward compatibility reasons (some people do use it).

I use them as well e.g. as Cursor ....

As for the line drawing, I have also thought about these questions and I think I'm opposed to even the single additional thickness parameter. It ....

Thats definetly a valid argumentation.

drawing a line from integer coordinates to integer coordinates should render something that is aligned with physical pixels, not ±0.5px (blurry).

Not sure how I can handle this, because (at least in my tests) if draw a quad with only 2 points I get no output at all. Thats why the angle/offset_x/y stuff made it in the code in the first place. And since I needed to calculate new coordinates is was no big step to make the variable publicly available. For now I would only went back to the apply the equivavalent of 1px to (x,y) to the "right" of the line.

[jlnr]

I use them as well e.g. as Cursor ....

Oh triangles are great, but they can also be implemented using draw_quadquite easily :)

Yeah...making draw_line align with the pixel grid in all cases sounds a bit odd. We should take a look at how the current draw_line works, which does this (I think–I haven't had a chance to do more than read GitHub today)

[Kjarrigan]

Yeah...making draw_line align with the pixel grid in all cases sounds a bit odd. We should take a look at how the current draw_line works, which does this (I think–I haven't had a chance to do more than read GitHub today)

I changed the circled line test to draw 1px lines with and without the PR and they are pretty close but not perfectly identical. See the attached Gif-Animation. The slightly "thicker" lines are from the PR.

So I guess you have the final word:

• try to make it match perfectly or is this good enough
• remove the thickness option
• keep the thickness but make them as hash params together with z: 0, mode: :default, thickness: 1.0)

[Kjarrigan]

I think–I haven't had a chance to do more than read GitHub today

Oh and don't let GitHub get in the way of your real work I can wait 😄

[Kjarrigan]

Wow. Time flies. Has it really been over a month since last post.... I guess the good weather doesn't help - its hard to get motivated to stay inside coding when you finally can do something outdoors again 😄

So I guess you have the final word:

try to make it match perfectly or is this good enough
remove the thickness option
keep the thickness but make them as hash params together with z: 0, mode: :default, thickness: 1.0)

Did you have a chance to look/think over at the "remaining" point of this PR?

[jlnr]

Oh sorry, I didn't realize that you were waiting for me here. Thanks for nudging me! I tend to ignore everything else when my own PRs get stuck 😅

I really need to play around with draw_line to see which behavior makes sense. As for the named arguments, I think that mode: :default is almost a must-have because that's what Image#draw accepts in #443. I'm not sure about z:. X and Y are never optional, should Z be?

If we merge this before #443, then I think we should not introduce keyword arguments yet, and we should also not introduce a non-keyword thickness argument only to turn it into a keyword argument in the next version. I also still feel that thickness is a concept that belongs in a separate Canvas-like class/library which can be built on top of Gosu's public interface. Better to have one less thing in Gosu that needs to be documented.

[jlnr]

Here's a draw_line rectangle starting at (2, 2) on the current master branch, on a 200% HiDPI screen:

Several problems with the existing implementation:

1. Line width depends on whether I'm running a HiDPI/Retina screen
2. Line ends don't match up
3. The rect doesn't even start at (2, 2)!

OK. Never mind compatibility with the previous implementation... 😅

Another problem that I've encountered is that quads are always aligned on the physical pixel grid right now, which is barely okay for images, but ruins thin lines. -> I've gone ahead and removed the thickness parameter, because I really think a good draw_line implementation would need to work in a different way. Or maybe Gosu should always enforce anti-aliasing?

I've rebased this branch, will update the test images and then hopefully merge it.

[jlnr]

Umm, great. :D The tests don't run on my HiDPI machine because the screenshots can't possibly match. It seems that the screenshot method also doesn't take the resolution into account, and only screenshots the lower-left quarter of my window. Turns out this automagic scaling has its pitfalls...

Good thing that I've planned to play around with screenshot a bit today!

[Kjarrigan]

👍 Nice seeing some progress. I'm working for a new company since March 1st so I'm currently a bit overwhelmed by all the new stuff the learn and to remember so it might take some more time until I can "come" back to work at my open PR's. Sorry for that.

[jlnr]

No problem, my text PR is also stuck because I'm working on several client projects at the same time right now :) Good luck with the new project.