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

Fix TextureStorage not assigning default scale #83199

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 12, 2023

While I don't know the exact conditions to cause it, I've had builds occasionally fail when processing the _render_target_get_sdf_rect functions, citing a value wasn't initalized (default switch case did nothing). The issue was fixed by mirroring the switch implementation in _render_target_allocate_sdf for the two relevant files

@Repiteo Repiteo requested a review from a team as a code owner October 12, 2023 15:23
@AThousandShips AThousandShips added this to the 4.2 milestone Oct 12, 2023
@AThousandShips
Copy link
Member

I think this should be fixed by ensuring the value is valid, and have an error in the default case as it's not a correct state

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 12, 2023

I would be fine with that route, but I was mirroring the implementation of another function that also handles scale. Would that mean the error case should be applied there as well?

@AThousandShips
Copy link
Member

AThousandShips commented Oct 12, 2023

That case doesn't cover all the values though it looks like

Or wait it's a different enum

@AThousandShips
Copy link
Member

I think it's more important to identify how an invalid value could have gotten in, that indicates problems, and shouldn't just be patched over

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 12, 2023

It'd be more involved to try and replicate the scenario that caused it, but that's a very valid point. It was happening often (exclusively?) when experimenting with double-precision int structs (godotengine/godot-proposals#7210), so I'll try to work backwards from there

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 12, 2023

Alright, figured it out! I recreated the scenario by making Vector2i use int64_t instead of int32_t and this was the failing output:

D:\Godot\Github\godot\drivers\gles3\storage\texture_storage.cpp(2320) : error C2220: the following warning is treated as an error
D:\Godot\Github\godot\drivers\gles3\storage\texture_storage.cpp(2320) : warning C4701: potentially uninitialized local variable 'scale' used

So line 2320 was the culprit, not strictly because a default case was passed, but because it was using the Vector2i * operator against an int when it was expecting an int64_t. So even though there's an implicit conversion between int32_t and int64_t, this only appears to take effect in a context where int32_t was initialized. Changing "scale" to int64_t fixed the issue, even when uninitalized

So while the proposed change in this PR is technically appropriate, it wasn't actually reaching that stage in practice. Instead, I'll shift direction to your initial proposal and assign an error to the default cases

• Print errors if an invalid value is passed
@Repiteo Repiteo force-pushed the texture-storage-default-scale branch from 3b911ad to 9ee41c7 Compare October 12, 2023 17:03
@akien-mga akien-mga merged commit 64f8029 into godotengine:master Oct 24, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the texture-storage-default-scale branch October 24, 2023 13:53
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