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

Make some Image methods static #63332

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 22, 2022

Follow-up to #60739 (comment)

Comment on lines 2068 to 2072
void Image::set_data(int p_width, int p_height, bool p_use_mipmaps, Format p_format, const Vector<uint8_t> &p_data) {
create(p_width, p_height, p_use_mipmaps, p_format, p_data);
}

void Image::create(int p_width, int p_height, bool p_use_mipmaps, Format p_format) {
Copy link
Member

@akien-mga akien-mga Jul 27, 2022

Choose a reason for hiding this comment

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

I think this warrants further refactoring, it's weird to have static create_empty and create_from_data and non-static create with two variants.

Maybe:

  • Move create(int p_width, int p_height, bool p_use_mipmaps, Format p_format) logic to create_empty (and rename to create to match bindings)
    • Or keep the create_empty name and use that for the bindings too. We should strive to have consistent between core C++ API and bindings for GDExtension's sake.
  • Move create(int p_width, int p_height, bool p_use_mipmaps, Format p_format, const Vector<uint8_t> &p_data) logic to set_data and use it in create_from_data.

reduz
reduz previously approved these changes Aug 8, 2022
@reduz
Copy link
Member

reduz commented Aug 8, 2022

I misunderstood what you were doing. I am not entirely sure its good to make these static, since they are operating on the image itself.

@reduz reduz dismissed their stale review August 8, 2022 18:11

Changed my mind

@akien-mga
Copy link
Member

I misunderstood what you were doing. I am not entirely sure its good to make these static, since they are operating on the image itself.

It's only the create methods which are static, I think that makes sense? It creates and give you a new Image.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 8, 2022

I think the internal create() method could be renamed to create_data(), because that's what it actually does.

@KoBeWi KoBeWi requested review from a team as code owners September 13, 2022 21:44
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 13, 2022

I renamed the internal create() to initialize_data() (I didn't use create_data() to avoid confusion with create_from_data()) and also replaced remaining old calls with static methods where applicable.

core/io/image.cpp Outdated Show resolved Hide resolved
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.

We discussed this in a PR meeting.

The change seems good, but we're concerned that it will lead to more problematic situations where users are unaware that their code is no longer working when using the previous format (which would still work silently).

We should find a proper solution to this UX issue in GDScript before merging more such changes that will impact users of the beta version.

@akien-mga akien-mga merged commit dc4b616 into godotengine:master Oct 15, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the static_images_aka_photos branch October 15, 2022 11:06
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