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

Adds back previous ParticleEffect API methods and minor convention chang... #2023

Merged
merged 2 commits into from Jun 26, 2014

Conversation

@obigu
Copy link
Contributor

obigu commented Jun 23, 2014

Commit d1bc56a has made some changes to ParticleEffect API and has been implemented in a way that, in my opinion, shouldn't have been merged.

I have made some changes to bring back the previous API methods not to break backwards compatibility (plus some minor changes) but I believe the whole thing should be rethought. I may have missed the idea but I'm not sure what it tries to solve.

There doesn't seem to be any documentation relating this convention either.

if (atlasPrefix.length() > 0) imageName = atlasPrefix + "/" + imageName;
@badlogic

This comment has been minimized.

Copy link
Member

badlogic commented Jun 25, 2014

Can you state what the problem is?

Edit: i see. What @MobiDevelop said.

@badlogic badlogic added the bug label Jun 25, 2014
@MobiDevelop

This comment has been minimized.

Copy link
Member

MobiDevelop commented Jun 25, 2014

I guess the problem is that the previous load and loadEmitterImages methods were removed, rather than overloaded. I would agree that they probably should have been overloaded with sensible defaults rather than replaced. Also, as was done here, I'd favor null as the non-value for atlasPrefix as opposed to an empty String. With the change I proposed above, I'd merge this.

@obigu

This comment has been minimized.

Copy link
Contributor Author

obigu commented Jun 25, 2014

Yes, what @MobiDevelop says, I should have made it clearer.

The other small issue I have is that the use of atlasPrefix is not documented in ParticleEffectLoader. The implementation is done in ParticleEffect class and the prefix is appended to "/".

In my opinion, atlasPrefix should contain all the characters to prefix to the imageName, including "/" if desired, leaving the choice to the developer.

If you agree, I will create a new PR with this last thing as well as @MobiDevelop suggestion.

@badlogic

This comment has been minimized.

Copy link
Member

badlogic commented Jun 25, 2014

agreed
On Jun 25, 2014 4:43 PM, "Oscar Bigu" notifications@github.com wrote:

Yes, what @MobiDevelop https://github.com/MobiDevelop says, I should
have made it clearer.

The other small issue I have is that the use of atlasPrefix is not
documented in ParticleEffectLoader. The implementation is done in
ParticleEffect class and the prefix is appended to "/".

In my opinion the prefix should contain all the characters to prefix to
the imageName, including "/" if desired, leaving the choice to the
developer.

If you agree, I will create a new PR with this last thing as well as
@MobiDevelop https://github.com/MobiDevelop suggestion.


Reply to this email directly or view it on GitHub
#2023 (comment).

@obigu

This comment has been minimized.

Copy link
Contributor Author

obigu commented Jun 25, 2014

Ok, made the change, ready for merging. Removed the appended "/" as discussed.

Now there's no need to check for empty String anymore so @MobiDevelop concerns are also solved with this last change.

@@ -173,7 +181,7 @@ public void loadEmitterImages (TextureAtlas atlas, String atlasPrefix) {
String imageName = new File(imagePath.replace('\\', '/')).getName();
int lastDotIndex = imageName.lastIndexOf('.');
if (lastDotIndex != -1) imageName = imageName.substring(0, lastDotIndex);
if (atlasPrefix.length() > 0) imageName = atlasPrefix + "/" + imageName;
if (atlasPrefix != null) imageName = atlasPrefix + imageName;

This comment has been minimized.

Copy link
@obigu

obigu Jun 25, 2014

Author Contributor

Now if atlasPrefix equals "", it doesn't matter anymore, no need to validate

@badlogic

This comment has been minimized.

Copy link
Member

badlogic commented Jun 26, 2014

Thanks. Adding a note to changes.

badlogic added a commit that referenced this pull request Jun 26, 2014
Adds back previous ParticleEffect API methods and minor convention chang...
@badlogic badlogic merged commit dc4bbca into libgdx:master Jun 26, 2014
@FredGithub

This comment has been minimized.

Copy link
Contributor

FredGithub commented Jun 28, 2014

@obigu for information on why this has been done, i invite you to read the corresponding pull request #1922

Its not a feature, it's more a temp fix waiting for the refactor of the 2d particle system

@obigu obigu deleted the obigu:fix2 branch Jun 28, 2014
@obigu

This comment has been minimized.

Copy link
Contributor Author

obigu commented Jun 28, 2014

Thanks @FredGithub, I can see the benefit of decoupling the name of the image from the name of the atlas region.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.