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 shader setter in SpriteText protected #6307

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

EVAST9919
Copy link
Contributor

Needed for my personal project.

@peppy
Copy link
Sponsor Member

peppy commented Jun 2, 2024

Hmm, usually we'd made a CreateTextureShader method and expose that instead. I'm not against just exposing as protected because it's very hard to misuse (you'd have to set it in the ctor to have it overwritten by framework).

At very least, xmldoc mentioning this is automatically set but can be overwritten post-load?

Will wait for another opinion though.

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

as commented

@bdach
Copy link
Collaborator

bdach commented Jun 3, 2024

Hmm, usually we'd made a CreateTextureShader method and expose that instead.

Sprite doesn't do this and it already has consumers of its protected setter:

public IShader TextureShader { get; protected set; }

As to whether it should, I guess I have no big feelings about that, but arbitrary setter allows for doing more (aside from the pitfall of setting it in ctor).

you'd have to set it in the ctor to have it overwritten by framework

At very least, xmldoc mentioning this is automatically set but can be overwritten post-load?

xmldoc can be added for sure. Though attempting to procure an IShader instance in a ctor would be mighty annoying I'd imagine so you could argue the API inertia here should already prevent that pitfall. But I agree that mentioning it (in both places maybe even) would not hurt.

@peppy peppy self-requested a review June 3, 2024 14:13
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Jun 3, 2024
@bdach bdach merged commit 8211a94 into ppy:master Jun 4, 2024
21 checks passed
@EVAST9919 EVAST9919 deleted the sprite-text-shader branch June 4, 2024 15:51
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.

3 participants