Add Image.draw(options) and tests (and remove draw_rot) #443

Kjarrigan · 2018-03-22T08:27:54Z

sorry for another PR but while looking through ashton and working on my little gosu game I stumbled across some things with the images.... Especially the long argument list bothered me quite a while and now that I know the internals a bit better I just changed it 😄
Here one general question remains for me - was there a specific reason you coded the other options (e.g. from_text) in the SWIG-File instead of Ruby? The latter is much more compact and keeping the SWIG stuff smaller is a good idea I think. When we drop Ruby < 2.x support we can make this even shorter with the builtin named parameters (which do all kinds of checks like raising if you have a typo in your param name).

Some of the changes in test_helper are stolen from #433 so I'll rebase if either one is merge first (did you have a change looking at the open questions over there?)

The replace-blend mode was something which looked important for ashton and wasn't hard to add (and more blend modes are already requested #413) so I thought I just add it.

Finally I added tests for all the things (at least I hope I didn't miss anything - I just looked through libgosu.org/rdoc from top to bottom). To keep the repo small I tried to reuse as many images as possible but maybe I should look for a better image for the retro/tileable-scaling tests.

Kjarrigan · 2018-03-22T08:29:22Z

src/RenderState.hpp

@@ -37,14 +37,17 @@ struct Gosu::RenderState
        }
    }

-    void apply_alpha_mode() const
+    void apply_blend_mode() const


alpha_blend_mode confused me a bit the first time I looked through the files because you actually change not only the Alpha-Channel, so I renamed it.

Good point. I guess the whole enum should be named Gosu::BlendMode with BM_ADD etc. as its members... :/

Kjarrigan · 2018-03-22T08:40:14Z

test/test_image.rb

+      # should     4--3
+      # be:       /    \
+      #          1------2
+      @image.draw_as_quad(0, w, c, w, h, c, w-15, h/2, c, 15, h/2, c, 0, :default)


Not sure about this one - But the one vertex is always off? I tried:
1-2 | 3-4 | 1-2
4-3 | 1-2 | 3-4

Update - might be an Linux thingy. Because thats the only green system on CI - every other platform failed this test (with my wrong image)....

Shouldn't the first parameters be 0, h, c?

Kjarrigan · 2018-03-23T09:50:50Z

I added Image.from_blob - mainly for viewing the blob differences from the CI but I can imagine some cool images/textures could be generated from random strings as well. ( In the spirit of #198 ).

Also a small bugfix discovered by simplecov (the gl_tex_info test was never called)...

jlnr · 2018-03-25T17:56:22Z

Gosu/GraphicsBase.hpp

@@ -26,7 +26,9 @@ namespace Gosu
        //! to the old color's channels.
        AM_ADD,
        //! The color's channels will be multiplied with each other.
-        AM_MULTIPLY
+        AM_MULTIPLY,
+        //! The color will completly replace the current one.


*completely

What is this mode good for? I've thought about extending the AlphaMode enum to cover all of Adobe's blend modes because I'm familiar with them from Photoshop, but I'm not sure what AM_REPLACE could be used for.

(I can think of use cases when drawing into an RGBA buffer; Gosu::Bitmap::insert is basically AM_REPLACE. But when you are drawing to the screen, you can't replace the destination pixels with translucent pixels.)

jlnr · 2018-03-25T17:58:29Z

ext/gosu/gosu.i

+    static Gosu::Image* from_blob(const std::string& blob, int width, int height)
+    {
+        Gosu::Bitmap bitmap(width, height);
+        memcpy(bitmap.data(), &blob[0], width * height * 4);


Since we know how long blob is, we can perform a range check here instead of reading from possibly invalid memory.

jlnr · 2018-03-25T17:59:49Z

lib/gosu/compat.rb

+      color   = args[2] || opts.fetch(:color, 0xff_ffffff)
+      mode    = args[3] || opts.fetch(:mode, :default)
+
+      # Make draw_rot obsolete as the params are bascially the same besides the rotation-specific values


jlnr · 2018-03-25T18:00:57Z

rdoc/gosu.rb

+    # Returns an image from a binary string of packed RGBA values. (e.g. from (Image#to_blob))
+    #
+    # @param blob [String] a string of any length (preferably width * height * 4)
+    # @param width [Ingeger] the width of the resulting image


jlnr · 2018-03-25T18:01:10Z

rdoc/gosu.rb

+    ##
+    # Returns an image from a binary string of packed RGBA values. (e.g. from (Image#to_blob))
+    #
+    # @param blob [String] a string of any length (preferably width * height * 4)


Not just preferably! :)

jlnr · 2018-03-25T18:11:31Z

test/test_helper.rb

+    include TestHelper
+
+    def self.draw(w, h)
+      win = new(w,h)


+space between operators

jlnr · 2018-03-25T18:13:04Z

test/test_helper.rb

+    skip if runs_on_ci?
+  end
+
+  def runs_on_ci?


Ruby doesn't usually have an s at the end of verbs (e.g. equal?). run_on_ci?, on_ci?, simply ci?? Not sure

jlnr · 2018-03-25T18:31:20Z

Re Image.draw(x, y, z, options): I like it! The long argument list has been annoying for ten years(!) now, see this discussion thread: https://www.libgosu.org/cgi-bin/mwf/topic_show.pl?pid=14

tl;dr my plan was to build image.scale(5).rotate(5, 0.5, 0.5).draw(x, y, z) instead of using an arguments hash. The idea here was to make it clear in which order operations are applied (scale first, rotation afterwards), and also to implement it without any extra object allocations, because Ruby GC pauses used to be a big problem with Gosu.

However, Ruby supports named arguments now, and I don't think it's a good idea to fight the language design. So let's do it!

As for the details: I'm not sure if :center_x and :center_y shouldn't work even without :angle; maybe the presence of :angle should only change their default values from 0.0 to 0.5?

What do you think about having scale: as a shorthand for scale_x: and scale_y:? I guess a shortcut for center: should also be added for consistency then, even if it seems less useful.

jlnr · 2018-03-25T18:34:21Z

Re: Image.from_blob, are you aware of how Image.new(...) already supports RMagick images in place of the filename parameter? I've added it for consistency with the Image(Bitmap) constructors in the C++ API. But I think from_blob is actually better, because it's much more discoverable, and RMagick has never been a popular companion library for Gosu anyway, so it feels silly that all "blob input" has to pass through an RMagick-like interface.

So 👍 for an explicit from_blob method, but it should use the same code path as the RMagick-based constructor, including the support for float-based RGBA binary strings (I think this might have been added for the TexPlay gem?).

jlnr · 2018-03-25T18:35:50Z

Re: AM_REPLACE, I'm not convinced :) But I do agree with every bit of renaming from AlphaMode to BlendMode.

Kjarrigan · 2018-03-26T07:27:00Z

Ok so many response....Hope I don't miss anything 😄

tl;dr my plan was to build image.scale(5).rotate(5, 0.5, 0.5).draw(x, y, z) instead of using an arguments hash.

Didn't knew that. Definitely a considerable approach - a bit like the ActiveRecord-Relation chaining. This might be possible to add besides the parameter Hash which internally just converts it for the final draw.

As for the details: I'm not sure if :center_x and :center_y shouldn't work even without :angle; maybe the presence of :angle should only change their default values from 0.0 to 0.5?

You're right. Drawing an image with the given (x,y) as the center rather than the upper-left-corner should be a thing and then the center-shortcut makes sense. We could even add a bit of abstraction by providing keywords instead of float numers like center: :middle (or :upperleft, :lowerright, etc.).

What do you think about having scale: as a shorthand for scale_x: and scale_y:?

Absolutely.

All kinds of typos....

I'll fix these. I guess I should look to some spell-checking addon for my editor 😄

AM_REPLACE, I'm not convinced

I'm not totally sure how this exactly works as well but Ashton used this quite a lot when combining shaders (and had a hint that it has to be done "manually" because Gosu doesn't support it out-of-the-box). But I guess I just remove it for now until I figure out how all the bits are connected and then re-add it later if needed.

AlphaMode to BlendMode. + AM_ to BM_

You're right - I'll change them too.

Re: Image.from_blob, are you aware of how Image.new(...) already supports RMagick images in place of the filename parameter?

Yes and no. Thats what I tried first (in irb while trying to convert by AppVeyor dump to a real image) because I thought the conversion from RMagick to Gosu would use some kind of blob. But something like:

Foo = Struct.new :to_blob, :columns, :rows
Image.new(Foo.new(Filebinread('dump'), 100, 100)).save('realimage.png')

didn't work 😄

but it should use the same code path as the RMagick-based constructor, including the support for float-based RGBA binary strings (I think this might have been added for the TexPlay gem?).

I'll look into this.

jlnr · 2018-03-28T18:03:12Z

ext/gosu/gosu.i

@@ -658,6 +658,32 @@ namespace Gosu
        std::unique_ptr<Gosu::ImageData> image_data = $self->data().subimage(x, y, w, h);
        return image_data.get() ? new Gosu::Image(std::move(image_data)) : nullptr;
    }
+
+    %newobject from_blob;
+    static Gosu::Image* from_blob(const std::string& blob, int width, int height)


If Gosu::Image works (because it's default constructible now?), we should always just return images directly instead of working with pointers and %newobject.

jlnr · 2018-03-28T18:03:45Z

lib/gosu/compat.rb

+
+  alias _draw_rot draw_rot
+  def draw_rot(*args)
+    Gosu.deprecation_message("Image#draw_rot is deprecated in Gosu x.y, use draw with an options hash instead, see https://www.libgosu.org/rdoc/Gosu/Image.html.")


x.y = 0.14 :)

jlnr · 2018-03-28T18:04:27Z

lib/gosu/compat.rb

+
+      # Shorthand for scale_x: scale_y:
+      scale = opts.delete(:scale)
+      scale_x, scale_y = scale, scale if scale


scale_x should override scale, not the other way around

jlnr · 2018-03-28T18:04:52Z

lib/gosu/compat.rb

+          if DRAW_ALIGNMENTS.has_key?(center)
+            DRAW_ALIGNMENTS[center]
+          else
+            raise ArgumentError, "Unknown drawing alignmengt: #{center} should be a float or one of: #{DRAW_ALIGNMENTS.keys.join(', ')}"


jlnr · 2018-03-28T18:06:29Z

lib/gosu/compat.rb

+      angle = opts.delete(:angle)
+
+      raise ArgumentError, "Unknown options: #{opts.keys} should be either of [angle, center, center_x, center_y, color, mode, scale, scale_x, scale_y]" unless opts.empty?
+      if angle or center_x or center_y


I don't think it's intuitive that passing center_x: 0.5 also sets center_y: 0.5, but leaving out center_x uses center_y: 0

I still think changing the default value of center_x/center_y based on the presence of angle is more intuitive, if only slightly :)

You're right. Was a bit confused as I need to call draw_rot if any of the 3 is set because draw doesn't support center so I had to set angle to a default as well but I guess now I got it ;-) but not sure how you came to

out center_x uses center_y: 0

all defaulted to 0.5

It only defaults to 0.5 when angle or either center_... parameter is given. So:

image.draw x, y, z, center_x: 0.5 # => center_y = 0.5 (surprising) image.draw x, y, z # => center_y = 0.0 (OK)

jlnr · 2018-03-28T18:08:50Z

lib/gosu/compat.rb

+    :bottom => [0, 1],
+    :bottom_right => [1, 1],
+  }
+  def draw(x, y, z, *args)


I wonder if executing relatively much Ruby code in every Image#draw call slows down existing Gosu games. I guess we should release a 0.14.0-pre1 gem this time and rewrite Image#draw in C++ if this is really an issue

Ok my girls are sleeping againg.....So - you're right. Performance might really be an issue here as drawing is done for possibly thousands of things. I use Ruby for over 10 years now and it was always fast enough with pure Ruby code or existing gems but I tend to forget that this is not my game but the layer below which should be fast. I'll see if I can imitate what you already did with the other methods supporting options.

jlnr · 2018-03-28T18:15:07Z

rdoc/gosu.rb

+    # @return [Gosu::Image]
+    #
+    # @see Image#to_blob
+    def self.from_text(blob, width, height); end


*from_blob :)

jlnr · 2018-03-28T18:16:44Z

rdoc/gosu.rb

+    ##
+    # Returns an image from a binary string of packed RGBA values. (e.g. from (Image#to_blob))
+    #
+    # @param blob [String] a binary string with 1 byte per channel and pixel (RGBA) (width * height * 4)


I think I might have added that already - but the old image constructor also supported 4 byte per channel for floating point values, from_blob should als officially support it as it doesn't cost us much

jlnr · 2018-03-28T18:18:04Z

rdoc/gosu.rb

@@ -1038,7 +1050,7 @@ def clip_to(x, y, w, h); end
    #
    # @note Because the returned object is not a true image---it's implemented using vertex buffers and is not backed by a texture---there are restrictions on how it can be used.
    #
-    # @note The width and height of the returned object will be the same values you passed to {record}, regardless of the area you draw on. It is important to pass accurate values if you plan on using {Gosu::Image#draw_as_quad} or {Gosu::Image#draw_rot} with the result later.
+    # @note The width and height of the returned object will be the same values you passed to {record}, regardless of the area you draw on. It is important to pass accurate values if you plan on using {Gosu::Image#draw_as_quad} or {Gosu::Image#draw with an angle set) with the result later.


The {braces} are broken now

jlnr · 2018-03-28T18:19:09Z

src/RenderState.hpp


    RenderState()
-    : transform(0), mode(AM_DEFAULT)
+    : transform(0), mode(BM_DEFAULT)


Can you please update this to transform(nullptr) while you're touching this line? Or even better, you can initialize the members inline using =

jlnr · 2018-03-28T18:22:44Z

A big PR but definitely the right direction 👍 Time for me to finally fix my own PR :)

... and combine it with draw rot.

jlnr · 2018-06-22T11:20:36Z

Regarding my previous comments on gosu.i:

If Gosu::Image works (because it's default constructible now?), we should always just return images directly instead of working with pointers and %newobject.

I take that back :) First, in some cases we need to work with pointers because methods can return nil in Ruby. Second, when I remove the pointer, it seems that SWIG generally wraps my method in new Image(...) to turn Gosu::Image into Gosu::Image* anyway, so we might as well keep doing it ourselves.

tl;dr %newobject Gosu::Image* is fine :) No need to mess with it.

Kjarrigan · 2018-07-12T06:10:24Z

@jlnr

Now that the screenshot is gone (#451) most of this PR could be removed I think, because the "tests" relied on it. So basically only keep the Renaming of AlpaMode -> BlendMode and drop everything else (maybe some of the RDoc fixes too)?

jlnr · 2018-07-12T07:27:52Z

@Kjarrigan Not at all! :) Window#screenshot is gone, but Gosu.render lives. So you should be able to retain all tests and have them work without the double-tick-and-wait hack, or without a window at all. The only bad news is that Gosu.render doesn't work on our Windows CI.

Kjarrigan · 2018-07-12T07:43:12Z

Ok. I can live with a not working windows, because some tests are better than none. I'll rework/rebase it later today and report back!

jlnr · 2018-09-05T12:25:38Z

Random idea: C++20 will support (a restricted form of) C99 struct initializers. I think all major C++ compilers already support this syntax as an extension. Why not use named arguments in C++ as well:

class Gosu::Font
{
...
public:
    struct DrawArgs
    {
        const std::string& text;
        double x, y, z = 0;
        double rel_x = 0, rel_y = 0;
        double scale_x = 1, scale_y = 1;
        Color color = Color::WHITE;
        AlphaMode mode = AM_DEFAULT;
    };
    void draw_text(DrawArgs args);
    ...

Then we should get named arguments as in Ruby:

    my_font.draw({ .text = "Hello World", .x = 5, .y = 7, .color = Gosu::Color::RED });

If this works on all CI compilers, we should totally do that, at least for all draw_* methods, create_text, and Sound::play.

Kjarrigan commented Mar 22, 2018

View reviewed changes

jlnr reviewed Mar 25, 2018

View reviewed changes

test/test_helper.rb Outdated

include TestHelper

def self.draw(w, h)

win = new(w,h)

Copy link

Member

jlnr Mar 25, 2018

Choose a reason for hiding this comment

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

+space between operators

jlnr reviewed Mar 25, 2018

View reviewed changes

Kjarrigan force-pushed the image-methods-options branch from 1049a13 to 67b09da Compare March 26, 2018 12:43

jlnr reviewed Mar 28, 2018

View reviewed changes

rdoc/gosu.rb Outdated

# @return [Gosu::Image]

#

# @see Image#to_blob

def self.from_text(blob, width, height); end

Copy link

Member

jlnr Mar 28, 2018

Choose a reason for hiding this comment

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

*from_blob :)

jlnr reviewed Mar 28, 2018

View reviewed changes

Kjarrigan added 4 commits March 28, 2018 21:45

Add tests for Image-class

fbe81be

Change Image#draw (long) argument list to an options Hash

ad20c11

... and combine it with draw rot.

Add Image.from_blob

b453abe

Rename AlphaMode -> BlendMode (& AM_ -> BM_)

6acbfbd

Fix minor rdoc typo

f4c536f

Kjarrigan force-pushed the image-methods-options branch from 67b09da to f4c536f Compare March 28, 2018 19:50

jlnr mentioned this pull request Apr 26, 2018

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

Open

jlnr mentioned this pull request Jul 12, 2018

Screenshot polish #451

Merged

3 tasks

jlnr force-pushed the master branch 3 times, most recently from 05ac06d to 46cd8fd Compare October 4, 2018 19:54

jlnr mentioned this pull request Nov 21, 2018

image_draw scale_x does not seem to work #477

Closed

jlnr force-pushed the master branch from da38bc1 to 813d852 Compare December 29, 2020 22:45

jlnr force-pushed the master branch from 5f9a3d8 to 6312a09 Compare March 20, 2021 22:04

Add Image.draw(options) and tests (and remove draw_rot) #443

Are you sure you want to change the base?

Add Image.draw(options) and tests (and remove draw_rot) #443

Conversation

Kjarrigan commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kjarrigan commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr commented Mar 25, 2018

jlnr commented Mar 25, 2018 • edited

jlnr commented Mar 25, 2018

Kjarrigan commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlnr commented Mar 28, 2018

jlnr commented Jun 22, 2018

Kjarrigan commented Jul 12, 2018

jlnr commented Jul 12, 2018

Kjarrigan commented Jul 12, 2018

jlnr commented Sep 5, 2018

jlnr commented Mar 25, 2018 •

edited

jlnr Mar 28, 2018 •

edited