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

Remove incorrect statement about TextureButton normal texture #95660

Merged

Conversation

Meorge
Copy link
Contributor

@Meorge Meorge commented Aug 16, 2024

The documentation currently states that TextureButtons must have a texture for their "normal" state in texture_normal, but I found this didn't seem to be the case in practice, as noted here: godotengine/godot-docs#9763

This PR simply removes that line about needing the texture from the TextureButton documentation page.

Bugsquad edit: fixes godotengine/godot-docs#9763

@Meorge Meorge requested a review from a team as a code owner August 16, 2024 21:31
@Chaosus Chaosus added this to the 4.4 milestone Aug 17, 2024
@AThousandShips
Copy link
Member

This depends I'd say what is meant, I think it should instead explain what happens without this texture and suggest using it unless you know what you're doing

@Meorge
Copy link
Contributor Author

Meorge commented Aug 17, 2024

That makes sense; how does something like this sound?

It is recommended to at least set a texture for the "normal" state ([member texture_normal]). Otherwise, the TextureButton will still receive input events and be clickable, but the user will not be able to see it.

I suppose I should test adding textures for texture_disabled, texture_focused, texture_hover, and texture_pressed to confirm how they interact with texture_normal (or a lack thereof) - from the docs it sounds like these textures may show up once the user performs the relevant interaction, and as such should factor into the description.

@Mickeon
Copy link
Contributor

Mickeon commented Dec 2, 2024

I would suggest you update the PR to add the note you wrote (or similar) at the bottom the leading description.
There's nothing explicitly preventing TextureButton from existing without texture_normal. But, if no corresponding texture exists, texture_normal is what the other button states fall back to. So, in a way, it is essential.

The vague statement being removed sucks, anyway. It leaves you with your own thoughts assuming the worst.

@Meorge Meorge force-pushed the remove-texturebutton-needs-normal-texture branch from 54bb17d to a7f82de Compare December 2, 2024 14:53
@Meorge
Copy link
Contributor Author

Meorge commented Dec 2, 2024

Thanks for the reminder! I've updated the PR with a version of what I'd written above. I wasn't entirely sure how to succinctly phrase the idea that the other states fall back to the "normal" state; hopefully the current wording is okay, but if anyone has suggestions for improving it, please feel free to add them in a review!

@Mickeon
Copy link
Contributor

Mickeon commented Dec 2, 2024

I wasn't entirely sure how to succinctly phrase the idea that the other states fall back to the "normal" state

Honestly I feel like that's something that each state texture property's description could be more explicit about, instead of summarising it in the leading description.

@Mickeon Mickeon requested a review from a team December 2, 2024 16:21
@Meorge Meorge force-pushed the remove-texturebutton-needs-normal-texture branch from d1766e9 to 06230dd Compare December 2, 2024 16:31
@Meorge
Copy link
Contributor Author

Meorge commented Dec 2, 2024

I pushed a few changes to elaborate more on how the texture-swapping works, both in the note at the beginning and the descriptions for the texture properties.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I would have heavily suggested doing this separately because this now requires a new round of fact-checking.

@Meorge
Copy link
Contributor Author

Meorge commented Dec 2, 2024

I created a small test project to verify these behaviors before I added the notes to the documentation - a little later today I can post a set of short videos demonstrating them in action

Edit: I've added videos and descriptions of the TextureButton behaviors as textures are assigned and unassigned. Notably, it did lead me to realize that we falsely stated texture_pressed would fall back to texture_normal, when in fact it falls back to texture_hover.

@Repiteo
Copy link
Contributor

Repiteo commented Dec 12, 2024

Is this ready as-is, or do these adjustments still need to be integrated?

@Meorge
Copy link
Contributor Author

Meorge commented Dec 12, 2024

Looks like I need to commit those suggestions and squash, and then it should be ready to go. I'll try to get that done within the next hour or two :)

…mal state in docs

Update doc/classes/TextureButton.xml

Co-authored-by: Micky <66727710+Mickeon@users.noreply.github.com>

Improve explanation of texture fallback behaviors for TextureButton

Remove a bit of incorrect information from TextureButton documentation

Most other state textures are not overlaid (only `texture_focused`)

Apply suggestions from code review
@Meorge Meorge force-pushed the remove-texturebutton-needs-normal-texture branch from 5611ed1 to 46c9089 Compare December 12, 2024 23:35
@Meorge
Copy link
Contributor Author

Meorge commented Dec 12, 2024

Updates are pushed and history is squashed!

@Repiteo Repiteo merged commit d7e4b9e into godotengine:master Dec 13, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 13, 2024

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.

TextureButton's "normal" state is falsely stated to require a texture
5 participants