godot icon indicating copy to clipboard operation
godot copied to clipboard

Expose `Texture2D::is_pixel_opaque()`

Open SourceOfHTML opened this issue 1 year ago • 1 comments

This resolves #98315 by binding the method, which was not bound previously.

Note; the issue specifically mentions ImageTexture as also having this problem, but when I bound both Texture2D's is_pixel_opaque() and ImageTexture's, Godot threw an error, so I went with only binding Texture2D's is_pixel_opaque().

I haven't checked every class that derives from Texture2D, but I have checked two, and they either had their own implementations of is_pixel_opaque(), or none at all, so those might be fine.

I am not very confident in anything, not especially my understanding of what I'm doing here, so if I've skipped a step or caused a regression somehow, please let me know, thank you for your time :)

SourceOfHTML avatar Oct 19 '24 19:10 SourceOfHTML

You need to update the Texture2D.xml file to include documentation for the method, see: https://docs.godotengine.org/en/stable/contributing/documentation/updating_the_class_reference.html#updating-class-reference-when-working-on-the-engine

If you don't have a way to easily compile the engine, you can instead apply the diff in the failing check as a basis: https://github.com/godotengine/godot/actions/runs/11420093268/job/31775421526?pr=98346

RedMser avatar Oct 20 '24 23:10 RedMser

Edit: OK, I looked through the source again to resolve my confusion with these functions.

It seems _is_pixel_opaque is already exposed through Texture2D.

https://github.com/godotengine/godot/blob/25dd96179ca38da52762000594c35ec30b22a5da/scene/resources/texture.cpp#L113

https://github.com/godotengine/godot/blob/25dd96179ca38da52762000594c35ec30b22a5da/doc/classes/Texture2D.xml#L69-L76

is_pixel_opaque is a c++ virtual function that, by default, calls its GDVirtual implementation:

https://github.com/godotengine/godot/blob/25dd96179ca38da52762000594c35ec30b22a5da/scene/resources/texture.cpp#L51-L55

Though in the case of ImageTexture it is overriden in c++ land:

https://github.com/godotengine/godot/blob/master/scene/resources/image_texture.cpp#L176-L207

I guess your fix of exposing the static function is therefore appropriate.

Ivorforce avatar Dec 04 '24 15:12 Ivorforce

Could you amend the commit message to be more explicit? Like this PR's title would be a better option, as it clarifies which class we're talking about. I'd normally do such edits myself but it seems maintainer pushes on this PR are disabled.

akien-mga avatar Jan 14 '25 10:01 akien-mga

I looked at what is_pixel_opaque does in the code and it seems a bit more complex than just a check against alpha. I'm concerned we're exposing something somewhat internal here.

QbieShay avatar Jan 14 '25 10:01 QbieShay

If it were entirely internal, surely we wouldn't list it in documentation, then? We could remove it from documentation too

Alternatively, if we're really worried about over-exposing, we could just re-write it from scratch, maybe?

SourceOfHTML avatar Jan 14 '25 10:01 SourceOfHTML

I looked at what is_pixel_opaque does in the code and it seems a bit more complex than just a check against alpha. I'm concerned we're exposing something somewhat internal here.

As far as I'm seeing it is currently possible to override, but not call the function. That seems a little bit odd to me?

Ivorforce avatar Jan 14 '25 11:01 Ivorforce

Just give us a little time to figure out whether this is indeed an oversight and should be exposed, or there's actually a reason why it's not exposed. This isn't against this PR in particular, I am just making sure when adding a new user API (and so close to beta) that we don't make a mistake, because taking back exposed API is problematic and breaks compat.

QbieShay avatar Jan 14 '25 11:01 QbieShay

This issue came up https://github.com/godotengine/godot/issues/57680#issuecomment-1031535760, will need further discussion

QbieShay avatar Jan 14 '25 23:01 QbieShay

I don't think we should merge this without a very clear need. Right now the use-case appears to be just consistency with the exposed GDVIRTUAL, since this has been exposed for overriding (when implementing your own Texture2D class), but not for calling.

is_pixel_opaque() is kind of a risky function to call. It isn't implemented by all texture types, and for many texture types it has an extremely high performance cost (particularly CompressedTexture2D). We shouldn't expose is_pixel_opaque() without making some effort to ensure that the results are consistent (and implemented) for all texture types and that the performance cost is either minimized or well documented. It isn't something that should just be exposed on a whim because it happens to already exist.

clayjohn avatar Nov 26 '25 15:11 clayjohn