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

Add static methods for creating Image and ImageTexture #60739

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 3, 2022

Implements #23782
Originally I said that it wouldn't be so useful, but then I actually had to use Image/ImageTexture a bit more. Whenever you create ImageTexture, you immediately assign an image. It gets old fast, so having a shorthand is helpful.

I didn't stop here though. Seems like ImageTexture.create_from_image() is used a lot in core, so I replaced all usage by the new static method. Diff numbers say it well how it simplifies the code.

What changed:

  • ImageTexture.create_from_image() is now static
    • seems like non-static version of this method is needed in core (it has a few uses), so I added set_image(). It's not exposed
  • Added Image.load_from_file(), which creates Image and loads data
    • I didn't replace load(), because sometimes it's necessary get the loading error
      • although I opened a proposal that would allow it

Now it's possible to e.g. assign a dynamically loaded texture in one line:

$Sprite.texture = ImageTexture.create_from_image(Image.load(path))

It saves lots of typing.

@KoBeWi KoBeWi added this to the 4.0 milestone May 3, 2022
@KoBeWi KoBeWi requested review from a team as code owners May 3, 2022 23:59
@KoBeWi KoBeWi force-pushed the add_static_methods_everywhere!! branch 2 times, most recently from 38939c0 to 54f63ae Compare May 4, 2022 00:14
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I use the image and image texture operators a lot too.

Comment on lines 656 to 658
Ref<Image> image;
image.instantiate();
image->create(w, h, false, Image::FORMAT_RGB8, img);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like another good candidate for a static method (in a follow-up PR).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Needs a rebase, otherwise looks great to me.
Hopefully the rebase also fixes the godot-cpp build which seemed to choke on the static method back in May.

@KoBeWi KoBeWi force-pushed the add_static_methods_everywhere!! branch from 54f63ae to d290042 Compare July 8, 2022 11:49
@akien-mga
Copy link
Member

Hopefully the rebase also fixes the godot-cpp build which seemed to choke on the static method back in May.

Seems not. CC @godotengine/gdextension

/home/runner/work/godot/godot/godot-cpp/gen/src/classes/image_texture.cpp: In static member function 'static godot::Ref<godot::ImageTexture> godot::ImageTexture::create_from_image(const godot::Ref<godot::Image>&)':
/home/runner/work/godot/godot/godot-cpp/gen/src/classes/image_texture.cpp:46:116: error: '_nullptr' was not declared in this scope; did you mean 'nullptr_t'?
   46 |  return Ref<ImageTexture>::___internal_constructor(internal::_call_native_mb_ret_obj<ImageTexture>(___method_bind, _nullptr, image->_owner));
      |                                                                                                                    ^~~~~~~~
      |                                                                                                                    nullptr_t

@bruvzg
Copy link
Member

bruvzg commented Jul 8, 2022

I can't test it right now, but it's likely caused by a typo on this line:

binding_generator.py#L1155

-method_call += f"return Ref<{return_type}>::___internal_constructor(internal::_call_native_mb_ret_obj<{return_type}>(___method_bind, _nullptr"
+method_call += f"return Ref<{return_type}>::___internal_constructor(internal::_call_native_mb_ret_obj<{return_type}>(___method_bind, nullptr"

@akien-mga
Copy link
Member

akien-mga commented Jul 8, 2022

I can't test it right now, but it's likely caused by a typo on this line:

binding_generator.py#L1155

-method_call += f"return Ref<{return_type}>::___internal_constructor(internal::_call_native_mb_ret_obj<{return_type}>(___method_bind, _nullptr"
+method_call += f"return Ref<{return_type}>::___internal_constructor(internal::_call_native_mb_ret_obj<{return_type}>(___method_bind, nullptr"

That looked like a copy paste mistake from the _owner on the next line.

I pushed the fix: godotengine/godot-cpp@bffedfe

@akien-mga akien-mga merged commit d26442e into godotengine:master Jul 8, 2022
@akien-mga
Copy link
Member

Thanks!

@RandomShaper
Copy link
Member

@KoBeWi, may you perhaps make Image.create() and Image.create_from_data() static too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants