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

Add FlxObject.defaultMoves field #2980

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Add FlxObject.defaultMoves field #2980

merged 5 commits into from
Dec 19, 2023

Conversation

VMan-2002
Copy link
Contributor

Simple static variable to change the default value of moves

@moxie-coder
Copy link
Contributor

moxie-coder commented Dec 11, 2023

this is just useless, it's already got a default value, why need a variable for that? and you can just use sprite.moves = false to change that or extend FlxObject to do that too

@Geokureli
Copy link
Member

Geokureli commented Dec 11, 2023

this is just useless, it's already got a default value, why need a variable for that? and you can just use sprite.moves = false to change that or extend FlxObject to do that too

because this is a static var that determines the default value of every FlxObject created, similar to FlxSprite.defaultAntiAliasing

flixel/FlxObject.hx Outdated Show resolved Hide resolved
@moxie-coder
Copy link
Contributor

moxie-coder commented Dec 11, 2023

this is just useless, it's already got a default value, why need a variable for that? and you can just use sprite.moves = false to change that or extend FlxObject to do that too

because this is a static var that determines the default value of every FlxObject created, similar to FlxSprite.defaultAntiAliasing

I mean, that’s great but this probably should be in FlxG like you suggested earlier since it would make it in line with the other code, in contrast with defaultAntialiasing

@Geokureli
Copy link
Member

Geokureli commented Dec 12, 2023

Having statics in FlxObject and FlxSprite is actually the current convention, in the future these might move to FlxG, and when that happens we can move this too. But its gonna take some planning before that happens so lets just make a static in FlxObject for now

@moxie-coder
Copy link
Contributor

Having statics in FlxObject and FlxSprite is actually the current convention, in the future these might move to FlxG, and when that happens we can move this too. But its gonna take some planning before that happens so lets just make a static in FlxObject for now

sounds good

@VMan-2002 VMan-2002 changed the title defaultMoves variable movesDefault (formerly defaultMoves) variable Dec 12, 2023
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.

almost all good, but one point still remains

Few notes:

Edit: looks like you fixed this

@Geokureli Geokureli changed the title movesDefault (formerly defaultMoves) variable defaultMoves (formerly movesDefault) variable Dec 12, 2023
flixel/FlxObject.hx Outdated Show resolved Hide resolved
@Geokureli
Copy link
Member

I notice this comment on moves:

FlxObject and FlxSprite default to true. FlxText, FlxTileblock and FlxTilemap default to false.

can you confirm that FlxText, FlxTilemap and FlxTileBlock default to false even when defaultMoves is true? In fact we should probably have unit tests that verify this

@Geokureli Geokureli added this to the 5.6.0 milestone Dec 13, 2023
@Geokureli
Copy link
Member

in the future, please do not make PRs to flixel using your fork's dev branch, create a new feature branch, so it's easier for others to test and make changes to it

@Geokureli
Copy link
Member

Geokureli commented Dec 19, 2023

tested this with the following code, it works fine

package states;

import flixel.FlxG;
import flixel.FlxObject;
import flixel.FlxSprite;
import flixel.text.FlxText;
import flixel.tile.FlxTilemap;
import flixel.tile.FlxTileblock;

class DefaultMovesTestState2980 extends flixel.FlxState
{
    override function create()
    {
        super.create();
        
        assertMoves(new FlxObject());
        assertMoves(new FlxSprite());
        assertNotMoves(new FlxText());
        assertNotMoves(new FlxTilemap());
        assertNotMoves(new FlxTileblock(0, 0, 1, 1));
        
        FlxObject.defaultMoves = false;
        assertNotMoves(new FlxObject());
        assertNotMoves(new FlxSprite());
        assertNotMoves(new FlxText());
        assertNotMoves(new FlxTilemap());
        assertNotMoves(new FlxTileblock(0, 0, 1, 1));
        
        FlxObject.defaultMoves = true;
        assertMoves(new FlxObject());
        assertMoves(new FlxSprite());
        assertNotMoves(new FlxText());
        assertNotMoves(new FlxTilemap());
        assertNotMoves(new FlxTileblock(0, 0, 1, 1));
    }
    
    function assertMoves(obj:FlxObject)
    {
        if (obj.moves == false)
            throw 'Expected obj: $obj, moves:TRUE, got FALSE';
    }
    
    function assertNotMoves(obj:FlxObject)
    {
        if (obj.moves)
            throw 'Expected obj: $obj, moves:FALSE, got TRUE';
    }
}

@Geokureli Geokureli merged commit 393641e into HaxeFlixel:dev Dec 19, 2023
16 checks passed
@Geokureli Geokureli changed the title defaultMoves (formerly movesDefault) variable Add FlxObject.defaultMoves field Jan 4, 2024
Geokureli added a commit that referenced this pull request Jan 4, 2024
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.

3 participants