Closed towerofnix closed 2 months ago
I will be gone for a few days for the 4th of July so if @adroitwhiz is happy with this (or any other PR) feel free to merge. Otherwise I'll look into it when I return. :)
Still on vacation but I have some time at a coffee shop. I created a test project to try this out, and I'm still experiencing some issues.
When I set the sprite size to NaN (0 / 0
), I get the following console warning, which is correct.
But the sprite becomes invisible, as if it's being rendered at 0% as opposed to the promised 100%.
Additionally, setting the sprite size to Infinity
and -Infinity
is broken. (Actually, setting the sprite size to be any super large finite number causes the project to crash in Chrome on my Mac.) I know dealing with Infinity
wasn't originally the goal of this PR, but it feels like maybe a good thing to do now? But if that belongs in a separate PR that's also fine.
Thanks for checking that!!
Okay, so it turns out the issue with NaN is that we actually perform the sprite.size / 100
operation in two places. I haven't asserted this but I believe both are key to rendering and they really should stay in sync with each other.
So we moved the math for getting a sprite size into Drawable
(and tidied up its logic a little), in a public getSpriteScale
function, which is so-named based on the const spriteScale = spr.size / 100
lines (in both Renderer
and Drawable
) that it effectively replaces. That's now reused where we'd first inserted the logic (in Renderer
).
This makes NaN actually render as 100%. Thanks for the demo project catching it!
I don't think we can super reasonably handle setting the sprite size to massive values in this PR, because that's part of the "cap to maximum and minimum sizes" logic that would belong as its own thing — but -Infinity
flat-out causes the same sort of error as NaN, yet doesn't get caught. We'll address that for sure!
And, I think it's reasonable to, within the same function, banish any negative number. Negative size is completely meaningless in Scratch. In Leopard it doesn't appear to "break" - the sprite just goes invisible - but it's not rendering the speech bubble either, which is weird.
Here's our thought:
Edit: ^ These updated checks are implemented in the commits below!
We've followed up with an issue for handling minimum/maximum size: #202
Related to leopard-js/leopard-mentors#4. Resolves #105.
The main substantive change here is that
Renderer.renderSprite
will now always treat sprites with NaN or non-numbersize
as being 100%. This isn't "right" per se - "set size to NaN" is equal to "set size to 0" - but sprites should never be size NaN anyway, and I figure "treating as 100%" is more clearly-broken behavior than "treating as literally invisible". (Matching Scratch exactly would mean setting the size to the minimum valid size for that sprite's current costume, which the renderer isn't suited to handle on its own.)Secondary changes include
warnings in thesome light refactoring forSpeechBubbleSkin.getTexture
andVectorSkin.getTexture
interfaces, which expect a non-NaN scale, andVectorSkin._getMipmap
andRenderer.renderSprite
.This doesn't fix the case project (leopard-js/leopard-mentors#4), but it does clarify the error from a generic
WebGL: INVALID_VALUE: texImage2D: no canvas
intoExpected a number, sprite TiledChevron size is NaN. Treating as 100%.
.