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 pixel_size property to Sprite node #27935

Closed
wants to merge 1 commit into from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 11, 2019

Closes #23145

Works as an alias for scale, as changing one updates the other.
image

@KoBeWi KoBeWi requested a review from a team as a code owner April 11, 2019 18:11
@isaacremnant
Copy link
Contributor

I was really confused by the term "pixel size" for a second, wouldn't "image size" be better?

@groud
Copy link
Member

groud commented Apr 11, 2019

Honestly I don't think this feature is needed. It's a one liner in GDscript, so I don't think it is worth the bloat in the sprite properties.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 12, 2019

Well, the PR is there. Related issue could be closed whether it's merged or not.

Although if by any chance it's going to be merged, I guess it should use another way to detect size change, other than subscribing to notification. I just didn't know how to do it properly, so any advice is welcome >.>

@reduz
Copy link
Member

reduz commented Apr 20, 2019

I also don't think this is very useful, you know the pixel size when you create the art and in Godot most 2D games are pixel perfect anyway.

@akien-mga
Copy link
Member

I'm also unsure how useful this is. If you need pixel level precision, chances are that you don't want to scale at all, or scale only by powers of two, so abstracting the scale behind a "pixel size" seems misleading.

Can we know more about the use case? The feature request only says "add this", but not why.

@Jummit
Copy link
Contributor

Jummit commented May 6, 2019

I'm also unsure how useful this is. If you need pixel level precision, chances are that you don't want to scale at all, or scale only by powers of two, so abstracting the scale behind a "pixel size" seems misleading.

Can we know more about the use case? The feature request only says "add this", but not why.

One issue I often face is that I have to resize my assets for Godot because they are too large, and resizing them with the scale would affect all the children. I think the better solution would be to add a local scale, that only scales one node. This could be used not only for sprites, but for all 2d nodes, which would be quite useful.

@timoschwarzer
Copy link
Contributor

I like @Jummit's idea, something like 'Self Opacity' and 'Opacity' but for scaling.

@CedNaru
Copy link
Contributor

CedNaru commented May 7, 2019

I don't think it's an issue about scaling. A local scale variable would not change the root of the problem.
Before any scaling, the size of the Sprite IS the size of the texture.
In pixel perfect/low res game, it is mandatory in order to avoid blurriness/deformation. But in high res 2D game, textures can be treated like in 3D environnement where you change the size of your texture to adjust performance/art quality.
Even if the size change is a factor of two, it can be tedious to change all scaling factor for the involved Sprite2Ds.
To be clear. The goal is an option that let users define a prescaling size that doesn't change when the texture size change.
The solution may be to have an option to choose between "automatic sizing" which depend on the texture (default behaviour in current Godot) and "manual sizing" which depend on a width and height parameters.

"Pixel size" is a misleading name in my opinion even if I understand that KoBeWi chosed it because the distance unit in Godot 2D is the pixel.

@timoschwarzer
Copy link
Contributor

@CedNaru Ah, you want something like a target size? Like "always render that Sprite at X by Y pixels, regardless of texture size"?

@CedNaru
Copy link
Contributor

CedNaru commented May 7, 2019

@timoschwarzer Exactly!
But "Always render" is not a good expression because it would still be affected by the scale factor.

@timoschwarzer
Copy link
Contributor

timoschwarzer commented May 7, 2019

I think that's what TextureRect does with stretch_mode, with the overhead/disadvantage of being a Control node.

@CedNaru
Copy link
Contributor

CedNaru commented May 7, 2019

TextureRect is a Control Node and it misses a lot of options that Sprite2D have.
Edit: You edited while I was commenting

@CedNaru
Copy link
Contributor

CedNaru commented May 7, 2019

A even simpler option would be to have a boolean like "Keep the same apparent size when changing texture".
It would change the scaling automatically in this case.
We lose a more explicit control with a width and height property but don't bloat the Sprite inspector.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 7, 2019

I could change the "sprite size" to a property that enforces certain size when set above 0, if that would be more useful.

@eon-s
Copy link
Contributor

eon-s commented May 7, 2019

This sounds more like a Texture importing option than sprite property.

@CedNaru
Copy link
Contributor

CedNaru commented May 7, 2019

We are talking about enforcing the size of the sprite, not the size of the texture.
In the current version, you can partially control the size of the texture in the import options but it's only a max size limit.
Even if an import option for enforcing a specific texture size is added. It would mean that two different Sprites with the same texture can't have different target size.
On the other hand, the issue we discussed applies to all 2D nodes using textures, not only Sprite2D. So an import option would resolve globally this issue without the need to directly modify the source code of several nodes.

@akien-mga akien-mga removed the request for review from a team May 28, 2019 10:02
@akien-mga
Copy link
Member

Reading the above discussion, it does seem like the original request from #23145 is not particularly useful. Some further use cases have been outlined which are not covered in this PR:

  • The possibility to have a Sprite with a pre-defined size that will not change when changing the texture (thus auto-adjusting the scale if the texture changes). That's quite a corner case IMO, but also trivial to do with a tool script or a custom node extending Sprite. You can just export a fixed_size variable and set the scale to match it as needed.

  • Allowing to scale / resize a texture without impacting the children of a Sprite node, as some kind of "Self Scale". IMO this could introduce a lot of complexity in all math and physics calculation that depend on the scale. If the use case is valid, this could likely be done via a texture import option to set a target texture size, so that the imported texture is resized during import and will then be used with its new target size.

@KoBeWi KoBeWi deleted the pixel_size! branch November 25, 2022 15:09
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.

Editor: Provide Inspector property settings for size Sprite with pixel size