HaxeFlixel / flixel

Free, cross-platform 2D game engine powered by Haxe and OpenFL
https://haxeflixel.com/
MIT License
1.97k stars 434 forks source link

Permit centering a sprite at its origin on a flxpath #3052

Closed 47rooks closed 7 months ago

47rooks commented 7 months ago

Currently when a FlxSprite follows a FlxPath it cannot center itself on the path at its center of rotation. This can lead to some odd visuals in tilemaps or when the path itself is visible. This change attempts to maintain backward compatibility with the existing mechanism for choosing the center point while providing a new way to control the center point. This would support eventual deprecation of the autoCenter property in the future.

47rooks commented 7 months ago

These failures are on the nightly and look the same as those George posted about in discord a few days ago. Let me know if I need to do anything but for now it seems there is nothing for me to do on it.

Geokureli commented 7 months ago
  1. CI is failing because openfl's asset system isn't compatible with nightly haxe, no worries there
  2. This is a great change in the right direction, but I think we should go about it in a different way, It's a bit complicated to explain so I may just make some commits
47rooks commented 7 months ago

I'm happy either way so long as I can use the sprite origin to align the sprite on the path. Let me know when you have a fix and I can test it out.

Geokureli commented 7 months ago

would you be able to test this using whatever you were using before, and also test the new sprite.path.centerMode = CUSTOM(sprite.origin); as well? Eventually I'll test this on the pathfinder demo, too

47rooks commented 7 months ago

I just tested this and it works fine in both my programs. CUSTOM is a nice touch. I was thinking about arbitrary placement but had not lit on this idea. It works with sprite.origin but I'd just use ORIGIN but it's fine with FlxPoint(-10, -10) for example too.

Geokureli commented 7 months ago

i still have a test I wanna do but before i do that, @47rooks , what do you think of just having a path.centerOffset point, and removing the the centerMode enum all together?

My issues: it doesn't update to the new center when the object's size changes, so maybe null defaults to center and 0,0 is top left, and if you want it to center on the origin, you'd need to use centerOffset.copyFrom(origin), rather than using the origin instance, and change whenever origin changes, it, otherwise origin can be destroyed when the path is destroyed since path should destroy it's points, right?

trying to say it all out loud makes me think its a bad idea, but maybe I'm overthinking it?

47rooks commented 7 months ago

I read this probably while you were still typing it and gave an incomplete initial response. Here's a fuller one. It seems like more work for the caller. But it seems like there is also an issue where, like hitboxes, it cannot be automatically kept accurate when sprites are scaled. I'm not following why the origin of the sprite would be destroyed when the path is - unless it's got a reference to the sprite origin. In the current impl it doesn't have a reference because _point.add() just adds the values of the origin point. Oh I see what you mean now. If you go to a model where someone just sets and offsetpoint property someone could easily pass in a reference to the sprite's origin. And yeah that would likely be bad.

So if you created a path.centerOffset you would then need to make that a property and in the setter use copyFrom to protect the caller. That would be safe. Still more work for the user - flexible enough to cover all cases though. But either way the scaling issue is left as an issue for the caller to fix after scaling.

Geokureli commented 7 months ago

I think there was a miscommunication, due to me rambling on and thinking out loud. I wanted to entertain the idea of just having an centerOffset value rather than the the enum, the issues I describe above were not issues with the approach used in this PR, but issues with that new idea. I just thought I should bring it up in case you had input, but I think the current change is the way to go.

I'll have this merged in a little bit