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

Remove FlxPathManager, let FlxObject take care of updating it (#1087) #1712

Merged
merged 2 commits into from
Feb 9, 2016

Conversation

Gama11
Copy link
Member

@Gama11 Gama11 commented Feb 1, 2016

This takes care of any substate issues with paths still being updated while the object they belong to isn't. This solution works because every object can only follow one path at a time - for timers and tweens a different solution is needed. Similar to the original AS3 Flixel API, FlxObject now has a public path property.

Downside: it introduces a circular dependency between FlxObject and FlxPath.

…ixel#1087)

This takes care of any substate issues with paths still being updated while the object they belong to isn't. This solution works because every object can only follow one path at a time - for timers and tweens a different solution is needed. Similar to the original AS3 Flixel API, FlxObject now has a public `path` property.

Downside: it introduces a circular dependency between FlxObject and FlxPath.
if (object == null)
return;

if (_firstUpdate)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a terrible hack, but I couldn't think of a better solution right off the bat now that start() doesn't have an Object parameter anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

A better approach would be to have @:allow(flixel.util.FlxPath) _immovable in FlxObject and @:allow(flixel.util.FlxObject) _desiredImmovable in FlxPath, and have a public property that users would use to manually modify these values, like so:

public var immovable(get, set);
public inline function get_immovable() 
{
    return _immovable;
}
public function set_immovable(val) 
{
    if(path != null && path.active)
        return path._desiredImmovable = val;
    return _immovable = val;
}

The FlxPath would need to set _immovable directly, instead of using the property to avoid overriding the user value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'm not sure that approach is better. It leads to an even tigther coupling between FlxPath and FlxObject (accessing path._desiredImmovable), I already don't like it as is..

Copy link
Contributor

Choose a reason for hiding this comment

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

I is convinced, ship it. 👍

@gamedevsam
Copy link
Contributor

I think a better approach would be to migrate the managers to the FlxState class (when new Timer / Path / Tween object was created, it would get its manager from the active state). Everything has to be inside of a state anyways, so why not make each state contain its own set of IUpdatable plugins?

That way we could avoid instantiating the managers until they are needed (for example, currentState.timerManager could have a get property that would spawn the TimerManager if it was null). Still, any plugins would need to update before regular objects, to ensure order of operations wouldn't change.

I would prefer to address this problem for all plugins, not just for the path manager.

@Gama11
Copy link
Member Author

Gama11 commented Feb 1, 2016

I've thought about that. It's not really a solution to the underlying issue, it just prevents conflicts with substates. You might be manipulating the elapsed value of the super.update() call, and you'd expect the timers / tweens belonging to an object to slow down / speed up appropriately (the issue with that being that timers and tweens don't really belong to an object right now of course).

I think @JoeCreates' solution was essentially to add a lazily initialized manager to FlxBasic (which we could probably do without breaking changes if still keeping the old API available). Anyway, for FlxPath, a manager is overkill, since every object can only have one at a time.

return path;

if (this.path != null)
path.object = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably call path.destroy(); here instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, destroy() implies setting path to null and letting it be garbage-collected. Like I mentioned earlier, the user might want to restart() the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bug here:

if (this.path != null)
            path.object = null;

Should be:

if (this.path != null)
            this.path.object = null;

I understand your point about wanting to restart the path, I agree that we shouldn't make assumptions about how users want to handle path use / reuse, good point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@gamedevsam
Copy link
Contributor

I approve, 👍

@Gama11 Gama11 merged commit 4766403 into HaxeFlixel:dev Feb 9, 2016
Gama11 added a commit to HaxeFlixel/flixel-demos that referenced this pull request Feb 9, 2016
@Gama11 Gama11 deleted the FlxPathInObject branch May 11, 2016 14:41
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.

2 participants