ScenicFramework / scenic

Core Scenic library
Apache License 2.0
1.99k stars 137 forks source link

`Texture.clear!` only clears 1/3 of an `:rgb` texture #215

Closed shritesh closed 3 years ago

shritesh commented 3 years ago
System info:
Elixir: 1.11.4
Erlang: 23.3.1
Scenic: 0.10.3
OS: macOS 11.2.3 (20D91)
Arch: M1 / aarch64

https://github.com/boydm/scenic/blob/a87e6ff35bbc0b59251b77c3af94d037b2c43753/test/utilities/texture_test.exs#L332-L339

Adding the following test to the end fails:

assert Texture.get(tex, @width - 1, @height - 1) == {4, 8, 12}

Only the top third of the texture is being cleared. I have reproduced this issue with :ga and :rgba textures too.

The problem is with: https://github.com/boydm/scenic/blob/a87e6ff35bbc0b59251b77c3af94d037b2c43753/c_src/texture.c#L371

I have tested that removing the / 3 in the line fixes the issue.

boydm commented 3 years ago

Alright! Thank you for narrowing it down to the exact line of code with the problem. It should not be hard coding the pixel size to 3 bytes.

You want to take a crack at a PR? If I do it, I'll just roll it into the work on v0.11 (which is very far along now...)

boydm commented 3 years ago

Yeah. I see it now. It was accounting for the size of the pixels twice instead of once. The issue is that line 372 then skips up by 3 pixels, so the size should be left alone.

shritesh commented 3 years ago

Sure! I have done some of the work locally already to figure out the issue. I’ll submit a PR in the next day or two.

boydm commented 3 years ago

Oh man... looking through this. I see other things to fix. The whole hints thing is too complicated when it is just a clear value. Especially shouldn't be carrying it around in the data structure when it is better off as an explicit parameter.

This will probably end up in the (growing..) list of changes in v.11

boydm commented 3 years ago

I'm also rethinking the asset/cache system. Will be much cleaner. Will probably promote this texture library out of Utilities (where it feels like an afterthought - because it was??) and make it part of assets properly.