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 ImageLoader take bit field flags #64776

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Aug 23, 2022

While working on godotengine/godot-proposals#5034 I concluded that I need to pass more flags to the individual image format loaders from the import settings. Existing method signature supports force_linear, and half of the importers don't even use it. So I figured that instead of adding a new flag at the end of the method and growing its list of parameters — it's better to reuse the existing parameter, but convert it to a bit field.

This PR doesn't add anything new, just converts the parameter and introduces an enum. I guess that this is compat breaking for modules, but otherwise the classes aren't exposed, I don't think.

This work is kindly sponsored by the Dialogic project.

core/io/image_loader.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.

Makes sense to me.

@YuriSizov YuriSizov force-pushed the import-images-moar-flags branch from 386c81d to 672e9d6 Compare August 23, 2022 11:39
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Makes sense and code changes look fine to me.

@reduz
Copy link
Member

reduz commented Aug 24, 2022

This conflicts with #63594, though it seems kind of simple enough and we should be fine to merge if @Faless does not mind. I have pending fixing the binds for FileAccess.

@YuriSizov
Copy link
Contributor Author

Since this is ready, I'd prefer we merged 🙃 Shouldn't be hard for @Faless to rebase his PR and expose flags and all that.

@Faless
Copy link
Collaborator

Faless commented Aug 24, 2022

I have no problem rebasing my PR, but shouldn't this use BitField for the flags instead of uint32_t (see #62374)?

@YuriSizov
Copy link
Contributor Author

@Faless I've references some similar code in the resource saver, I think. I guess BitField is relevant if this is exposed, which it isn't yet, as of this PR?

@akien-mga
Copy link
Member

IMO this stuff could use BitField consistently but it doesn't have to be done in this PR, it does look like almost nothing is using BitField in core aside from core_bind.

@akien-mga akien-mga merged commit 0cf0e96 into godotengine:master Aug 25, 2022
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the import-images-moar-flags branch August 25, 2022 14:55
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.

6 participants