Skip to content

Fix Surface.draw_texture (#1705) #1769

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

Merged

Conversation

pushfoo
Copy link
Member

@pushfoo pushfoo commented May 11, 2023

Closes #1705

Changes

  • arcade.gui.Surface.draw_texture now raises NotImplementedError when alpha != 255 or angle != 0.
  • Added test

How to test

Run test suite as normal

Retained for context

I have the logic corrected, but there are two questions I need answered:

  1. Is support for position implemented? One comment says it is, while another gives me the impression it isn't. Updated per comments
  2. Is using the window fixture acceptable in tests/unit? If not, should I build out a new mock_window fixture to fully or partially mock the gl context + window? Implemented. I'd prefer to decouple this better, but it won't get us closer to 3.0.

@eruvanos
Copy link
Member

@pushfoo
hi,

to 1.

Please remove the exception, it is implemented and we should support it.

TL;DR

I see the confusion :D
NinePatchTextures do support positioning. So in a general sense, we would be able to use it to render the NinePatchTexture in some specific place.

The big drawback is, that in the normal flow, a widgets sets the viewport and scissor box to its rect.
In combination, any other value then 0,0 for position would mostly move it out of the scissor box.

So in general speak, the Exception is wrong, NinePatch does support a position, but we had the exception, because we wanted to prevent devs to use it in the wrong way.

So I change my opinion:
we should support this feature of NinePatchTexture and remove the overprotective restriction.

@eruvanos
Copy link
Member

Regarding 2.
Yes, window fixture is used in many places in unit test. You are good to go

@pushfoo pushfoo marked this pull request as ready for review May 11, 2023 21:47
Copy link
Member

@eruvanos eruvanos left a comment

Choose a reason for hiding this comment

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

Thanks

@eruvanos eruvanos merged commit 246abe1 into pythonarcade:development May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Fix Surface.draw_texture's NinePatchTexture handling
2 participants