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

Update FlxPoint pivotRadians to proper order of operations #2700

Merged
merged 4 commits into from
Dec 15, 2022

Conversation

MondayHopscotch
Copy link
Contributor

@MondayHopscotch MondayHopscotch commented Dec 15, 2022

Updates FlxPoint pivotRadians math which was calculating the rotation point improperly.
Fixes #2698

Old:

  1. subtract point from pivot
  2. apply rotation
  3. add pivot to result
  4. return

New:

  1. subtract pivot from point
  2. apply rotation
  3. add pivot to result
  4. return

I also updated the doc comments to specify "Counter-clockwise" as the use of cos and sin functions are counter-clockwise. Comparing behavior to the 4.11.0 branch, this is consistent with the behavior there despite that branch also specifying "clockwise"

@Geokureli
Copy link
Member

Geokureli commented Dec 15, 2022

@SeiferTim You mentioned in the discord that this doesn't fix #2698. It seems like there are multiple issues, here. to me this change makes sense but let me know what issues you're seeing, Tim

We're in a unique position here, anyone using the old p.rotate function will get a deprecation warning to use pivotDegrees, we can also use this to notify them of the new behavior, meaning this wouldn't be a breaking change, but I'd like to act fast, while people are still migrating to flixel 5

@Geokureli
Copy link
Member

also, fun fact I broke this when deprecating rotate and created pivotDegrees. so this change is definitely a good idea

Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be working completely as I would expect, everything is good except it should say "clockwise" instead of "counter-clockwise"

Screen.Recording.2022-12-15.at.2.19.29.PM.mov

as for Tim's concern:

he problem with the old method was simply that no matter where point is in relation to pivot when you call pivot*, it treats it as if it's at 0 degrees from pivot first...

I'm simply not seeing it, as evident by the example above, for which I'll provide the source:

package states;

import flixel.FlxG;
import flixel.FlxSprite;
import flixel.math.FlxPoint;

class PivotTestState extends flixel.FlxState
{
    var sprite:FlxSprite;
    var pivot:FlxPoint;
    var p:FlxPoint;
    
    override function create()
    {
        super.create();
        
        p = FlxPoint.get(0, 50);
        pivot = FlxPoint.get(50, 50);
        
        trace('$p @ ${(p - pivot).degrees)}');
        for (i in 0...8)
        {
            p.pivotDegrees(pivot, 45);
            trace('$p @ ${(p - pivot).degrees)}');
        }
        /* output (beautified):
         * (x:   0.000 | y:  50.000 ) @  180
         * (x:  85.355 | y:  85.355 ) @ -135
         * (x:  50.000 | y:   0.000 ) @  -90
         * (x:  14.645 | y:  85.355 ) @  -45
         * (x: 100.000 | y:  50.000 ) @    0
         * (x:  14.645 | y:  14.645 ) @   45
         * (x:  50.000 | y: 100.000 ) @   90
         * (x:  85.355 | y:  14.645 ) @  135
         * (x:   0.000 | y:  50.000 ) @  180
        **/
        
        pivot.set(FlxG.width / 2, FlxG.height / 2);
        var pivotSprite = new FlxSprite().makeGraphic(6, 6, 0xFF0000ff);
        pivotSprite.x = pivot.x - pivotSprite.origin.x;
        pivotSprite.y = pivot.y - pivotSprite.origin.y;
        add(pivotSprite);
        
        p.set(pivot.x + 100, pivot.y);
        sprite = new FlxSprite().makeGraphic(50, 50, 0xFFff0000);
        sprite.x = p.x - sprite.origin.x;
        sprite.y = p.y - sprite.origin.y;
        add(sprite);
    }
    
    override function update(elapsed)
    {
        super.update(elapsed);
        
        p.pivotDegrees(pivot, 360 * elapsed);
        sprite.x = p.x - sprite.origin.x;
        sprite.y = p.y - sprite.origin.y;
    }
}

@Geokureli Geokureli merged commit 9080aee into HaxeFlixel:dev Dec 15, 2022
Geokureli added a commit that referenced this pull request Dec 15, 2022
@MondayHopscotch MondayHopscotch deleted the 2698-FlxPoint-pivot branch December 15, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FlxPoint.pivotRadians is not working correctly
2 participants