FlxSpriteGroup clipRect support #2051
Conversation
flixel/group/FlxSpriteGroup.hx
Outdated
| @@ -879,6 +890,10 @@ class FlxTypedSpriteGroup<T:FlxSprite> extends FlxSprite | |||
| private inline function originTransform(Sprite:FlxSprite, Origin:FlxPoint) Sprite.origin.copyFrom(Origin); | |||
| private inline function scaleTransform(Sprite:FlxSprite, Scale:FlxPoint) Sprite.scale.copyFrom(Scale); | |||
| private inline function scrollFactorTransform(Sprite:FlxSprite, ScrollFactor:FlxPoint) Sprite.scrollFactor.copyFrom(ScrollFactor); | |||
| private inline function clipRectTransform(Sprite:FlxSprite, ClipRect:FlxRect) { | |||
There was a problem hiding this comment.
For the record, I don't like this part
There was a problem hiding this comment.
"This part"? Isn't this bascially the whole PR? :D
There was a problem hiding this comment.
I don't like how it looks so different from the others
There was a problem hiding this comment.
Aside from the inlined curly brace...
flixel/group/FlxSpriteGroup.hx
Outdated
| @@ -280,6 +281,9 @@ class FlxTypedSpriteGroup<T:FlxSprite> extends FlxSprite | |||
| sprite.alpha *= alpha; | |||
| sprite.scrollFactor.copyFrom(scrollFactor); | |||
| sprite.cameras = _cameras; // _cameras instead of cameras because get_cameras() will not return null | |||
|
|
|||
| if (clipRect != null) clipRectTransform(sprite, clipRect); | |||
| else sprite.clipRect = null; | |||
There was a problem hiding this comment.
This seems to make this a breaking change, because it removes the clipRect that sprite might have had before being added. Not sure that's a good idea?
There was a problem hiding this comment.
I guess the alternative would be to merge the parent's clip and the child's clip together. I'd have to look at that in Flash to see how it works. Either way, the child's clip would change, though.
There was a problem hiding this comment.
This is the case where the parent's clipRect is null though, so there isn't really anything to merge?
There was a problem hiding this comment.
I guess the "merge" would be null + child.clip which results in child.clip. Was your initial comment about line 286 or both of those lines?
There was a problem hiding this comment.
Yeah not sure why that line was there, removed.
Gama11
changed the title
|
The |
|
What I have works, but idk if I should add more to it or if it's correct. |
+ remove unnecessary line
MSGhero
changed the title
| @@ -280,6 +281,8 @@ class FlxTypedSpriteGroup<T:FlxSprite> extends FlxSprite | |||
| sprite.alpha *= alpha; | |||
| sprite.scrollFactor.copyFrom(scrollFactor); | |||
| sprite.cameras = _cameras; // _cameras instead of cameras because get_cameras() will not return null | |||
|
|
|||
| if (clipRect != null) clipRectTransform(sprite, clipRect); | |||
There was a problem hiding this comment.
This null check now seems unnecessary, since clipRectTransform() can handle null?
There was a problem hiding this comment.
You're right, that's some leftover code from when I was testing.
There was a problem hiding this comment.
Actually no, that would override the child's existing clip. The transform sets the child's clip based on the parent's, so you'd be setting null instead of leaving the existing one alone.
There was a problem hiding this comment.
Oh, you're right!
Great thing to unit-test. ;)
So are you still planning to write some tests for this? |
|
I guess I'll write some tests 😜 |
|
Kind of a weird test, but it covers the 3 cases: existing child clip, nonexisting child clip, and nonexisting parent clip. |
|
Thanks! I still hate how clipRects have to be re-assigned entirely for changes to be registered, but that doesn't really have anything to do with this PR... |
|
I don't think anyone likes that... |
|
Just occured to me, do we need to take offset into account? |
|
I want to say no, but I'll check. |
|
As it is now, it matches the behavior of a FlxSprite's clip+offset, where it clips first and offsets second (i.e. the offset doesn't affect where the clip occurs). One thing I noticed is that the parent's |
Needs more extensive (unit) testing, but the basics are done. Essential for haxeui components.
Suggestions welcome.