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

Allow disabling the alpha trim on texture atlas creation. #57163

Conversation

jasonwinterpixel
Copy link
Contributor

@jasonwinterpixel jasonwinterpixel commented Jan 24, 2022

When creating an AtlasTexture, godot automatically removes unused pixels from the borders of the images. This is unexpected behaviour that changes the size of the texture. In general, if a developer asks for a texture of a specific size with transparent pixels in it, that's what the developer should get. Changing the size and contents of the texture can cause a plethora of issues.

For our project, this broke all of our styleboxes, as they had region's specified which were invalid because the size of the texture changes.

This PR creates an option for AtlasTexture importers to allow developers to enable the trim on transparent pixels on the borders. It defaults to Off, which I think is better, more expected behaviour. [ Edit - now it default to On to maintain compatibility ]

Related: #53390

3.x version #57165

@kleonc
Copy link
Member

kleonc commented Jan 25, 2022

Fixes #53390

This PR doesn't really fix that, it only allows to force the AtlasTextures generated during the import to have zero margin (to not trim transparent border pixels). And the cause of #53390 is AtlasTexture::get_data() returning unexpected/incorrect data in case of non-zero margin, which this PR doesn't change whatsoever.
I mean this PR is fine on its own. If someone wants these transparent pixels to be included in the generated atlas then sure, let them have them. But that's only a way to omit / not face the #53390, not an actual fix.

@jasonwinterpixel
Copy link
Contributor Author

This PR doesn't really fix that

I understand your point. Maybe "fix" isnt the right word. But they are related and this PR implements that user's suggestion: "I suggest is godot trims transparent pixels from texture, need option to generate atlas as is with transparent pixels."

Also, would checking 'crop_to_region' also avoid the error that that user pointed out?

@kleonc
Copy link
Member

kleonc commented Jan 25, 2022

I understand your point. Maybe "fix" isnt the right word. But they are related and this PR implements that user's suggestion: "I suggest is godot trims transparent pixels from texture, need option to generate atlas as is with transparent pixels."

I just meant #53390 shouldn't be auto-closed by merging this PR (currently it will be because you wrote "Fixes #53390") as it doesn't fix the issue with the current state of things. But it's not a big deal, I guess another more specific issue could be open if that one gets closed (actually maybe I'll open one later anyway as there are more problems with the current implementation of AtlasTexture::get_data() than just non-zero margin case).

Also, would checking 'crop_to_region' also avoid the error that that user pointed out?

Yes, as with that option generated AtlasTextures have zero margin. But it would of course result in the generated AtlasTextures being smaller if there's actually some transparent border pixels originally.

Actually, didn't think about it earlier but with your addition the crop_to_region might be not working as expected anymore. When trim_alpha_border_from_region is false then the region which crop_to_region crops against will include transparent border pixels, and thus cropping to such region won't really crop transparent borders. Meaning crop_to_region has no effect when trim_alpha_border_from_region is false.
The thing is trim_alpha_border_from_region being false and crop_to_region being true are kinda mutually exclusive, so these options should probably be somehow merged into a single enum option instead? 🤔

@jasonwinterpixel
Copy link
Contributor Author

jasonwinterpixel commented Jan 25, 2022

Okay. I updated the comment to not say Fixes.

Whats the point of Crop To Region? Should it just be removed? What's the use case for having it checked and unchecked?

How does Crop To Region differ between Region and Mesh type import?

@kleonc
Copy link
Member

kleonc commented Jan 25, 2022

I mean I can gather as much info as you, have to look at the relevant PRs / proposals / code. 😉

Whats the point of Crop To Region? Should it just be removed? What's the use case for having it checked and unchecked?

Take a look at godotengine/godot-proposals#1661
It should generate AtlasTextures without the transparent borders, like there was no transparent borders in the first place. Meaning when cropping only "the center part" is saved in the atlas and margin is zero. By default (when not cropping) also only "the center part" is saved in the atlas but margin might be non-zero (so transparent border is saved as info about what it's like, pixels aren't stored directly as that's kinda wasteful).

How does Crop To Region differ between Region and Mesh type import?

Looking at the code, it's used only in Region mode.

@jasonwinterpixel
Copy link
Contributor Author

Thanks for the link.

So you want to see this as an enum where you either have Trim and Crop, Trim and Dont Crop or Dont Trim ?

@kleonc
Copy link
Member

kleonc commented Jan 25, 2022

So you want to see this as an enum where you either have Trim and Crop, Trim and Dont Crop or Dont Trim ?

Yeah, something like that. Just not sure if that's clear what does what so it should probably be somehow explained somewhere.

Also note that would be a breaking change so introducing such enum instead is probably not an option for 3.x.

@jasonwinterpixel
Copy link
Contributor Author

Good point. So maybe this is okay like this for 3.x ?

@Sousio
Copy link

Sousio commented Feb 4, 2022

This PR creates an option for AtlasTexture importers to allow developers to enable the trim on transparent pixels on the borders. It defaults to Off, which I think is better, more expected behaviour. [ Edit - now it default to On to maintain compatibility ]

I highly support this suggestion, as it can alleviate the user-defined code's overhead in retrieving the images by considering region and margin. While the current default setting of stripping out the transparent pixels seems to have a very low optimization effect on memory, it can be still an option. Overall I think one of the main usability problems with the Atlas Texture is that the user has very small control on the very step of creating one (just a few options when importing selected images as Atlas Texture), while it generates a lot of unnecessary data later on, hard to maintain and work with.

@jasonwinterpixel
Copy link
Contributor Author

This PR has 'breaks compat' tag on it, but I dont think this does break compat since the default behaviour is to keep the trimming.

Rect2 used_rect = image->get_used_rect();
Rect2 used_rect = Rect2(0, 0, image->get_width(), image->get_height());
if (trim_alpha_border_from_region) {
//clip a region from the image
Copy link
Member

Choose a reason for hiding this comment

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

While at it, the comment style could be improved too:

Suggested change
//clip a region from the image
// Clip a region from the image.

@jasonwinterpixel jasonwinterpixel force-pushed the feature/allow-disable-atlas-texture-alpha-trim branch from e224b4c to 31d723c Compare February 19, 2022 20:00
@akien-mga akien-mga merged commit 499eec1 into godotengine:master Feb 19, 2022
@akien-mga
Copy link
Member

Thanks!

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