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

Missing usage of particleClass in FlxEmitter #95

Merged
merged 6 commits into from Oct 28, 2012

Conversation

moly
Copy link
Member

@moly moly commented Sep 17, 2012

FlxEmitter now uses the custom particle class to recycle instead of FlxParticle.
Fixed #6, AdamAtomic#226

FlxEmitter now uses the custom particle class to recycle instead of FlxParticle.
Fixed FlixelCommunity#6, AdamAtomic#226
@IQAndreas
Copy link
Member

Looks good to me.

On a different note, you still run the risk of the user assigning an object that isn't a FlxParticle (there doesn't seem to be error handling for that, should there be?), but hopefully developers are smart enough to keep that in mind.

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

Looks ok to me too. If think it would be useful to log a warning if a class that isn't a FlxParticle is used. At least it will give the developer a clue when an exception pops up an ugly stack trace during the particles instantiation.

@IQAndreas
Copy link
Member

Is there a clever way to see if a Class object is of a certain type?

  • Create one instance and check it?
  • Parse it out of describeType()?

@moly
Copy link
Member Author

moly commented Sep 18, 2012

Easiest way is probably just to put a try catch around particle = new particleClass(); and particle = recycle

@IQAndreas
Copy link
Member

@moly Since you already have this pull request open, do you want to add that feature, or is it better to keep it separated as a different merge?

@moly
Copy link
Member Author

moly commented Sep 18, 2012

I'll just add it here.

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

It will produce several log entries. Shouldn't we create a setter for particleClass and there verify if the class is good? It will produce just a single log entry in case of problem.

Additionally I think the log message should display the problematic particleClass, someting like "error.." + particleClass + " more text". It will help the developer to find the malfunctioning emitter.

@IQAndreas
Copy link
Member

If I may (to compensate for me feeling so helpless in many of the other open pull requests):

protected var _particleClass:Class;
public function get particleClass():Class
{
    return _particleClass;
}
public function set particleClass(value:Class):void
{
    var testParticle:* = new value();
    if (testParticle is FlxParticle)
    {
        _particleClass = value;
    }
    else
    {
        FlxG.log("ERROR: " + getQualifiedClassName(testParticle) " must extend FlxParticle in order to be used in a FlxEmitter.");
    }
}

Adam doesn't use enough getters and setters, and I find that to be a shame.

@Dovyski
Copy link
Member

Dovyski commented Sep 18, 2012

Exactly what I had in mind, @IQAndreas.

Also moved around the error messages for said member.
@IQAndreas
Copy link
Member

@moly - Do you want to merge in my fix-flxemitter-particleclass branch into this pull request?
https://github.com/IQAndreas/flixel/tree/fix-flxemitter-particleclass

It includes the fixes we mentioned here.

@IQAndreas
Copy link
Member

@moly Hey, what did you do with my fancy getQualifiedClassName()? ;)

@IQAndreas
Copy link
Member

It might be a good idea also changing the ASDoc a tad more so as to resolve the issue #47

/**
* Set your own particle class type here. The custom class must extend <code>FlxParticle</code>.
* Default is <code>FlxParticle</code>.
*/

That way users don't need to trigger the error before learning about this restriction.

@Dovyski
Copy link
Member

Dovyski commented Oct 2, 2012

I agree (it should already be there :)

@IQAndreas
Copy link
Member

I just noticed Flixel has it's own function for retrieving the class name:

FlxU.getClassName(testParticle);

Could you add a commit with this change, and include the addition to the ASDoc?

@Dovyski
Copy link
Member

Dovyski commented Oct 15, 2012

I think it is useful. The complete class name will make things easier for developers to spot bugs.

@IQAndreas
Copy link
Member

@moly Great! I didn't realize you had already added the commits.

I didn't realize this before, but GitHub does not seem to notify when updates are added, only when comments are made. We should probably post a quick comment whenever we add or edit commits.

These changes look good to me, so I'll go ahead and merge them in.

IQAndreas added a commit that referenced this pull request Oct 28, 2012
Missing usage of particleClass in FlxEmitter
@IQAndreas IQAndreas merged commit 6cf6211 into FlixelCommunity:dev Oct 28, 2012
@IQAndreas IQAndreas mentioned this pull request Oct 15, 2013
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants