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

merged old and new PolygonRegionLoader #1665

Merged
merged 1 commit into from
Apr 9, 2014
Merged

merged old and new PolygonRegionLoader #1665

merged 1 commit into from
Apr 9, 2014

Conversation

dermetfan
Copy link
Contributor

Didn't want to break API with my second PR, but since you agreed in #1602, I do it in the third.

@badlogic
Copy link
Member

badlogic commented Apr 9, 2014

Curious, why would that break the API?

badlogic added a commit that referenced this pull request Apr 9, 2014
merged old and new PolygonRegionLoader
@badlogic badlogic merged commit 848f46a into libgdx:master Apr 9, 2014
@dermetfan
Copy link
Contributor Author

Some people might have used the old PolygonRegionLoader. Like for example the new PolgyonRegionLoader originally did. Since I wrote it for libgdx-utils, I couldn't just merge it with the existing one. If anyone did something similiar, it's now broken.

@MobiDevelop
Copy link
Member

I probably would have left it in its original location. Then it wouldn't have broken anything for anyone.

@badlogic
Copy link
Member

badlogic commented Apr 9, 2014

On 09.04.14 22:57, MobiDevelop wrote:

I probably would have left it in its original location. Then it
wouldn't have broken anything for anyone.


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

ya, moving it there.

@dermetfan
Copy link
Contributor Author

Keeping an AssetLoader in graphics.g2d instead of assets.loaders does not seem right to me though. That would mess things up IMO

@badlogic
Copy link
Member

badlogic commented Apr 9, 2014

On 09.04.14 22:59, Robin S wrote:

Keeping an AssetLoader in graphics.g2d instead of assets.loaders does
not seem right to me though. That would mess things up IMO


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

Nope, that's fine. Moving a class to another package breaking people's
code is messing things up.

@MobiDevelop
Copy link
Member

Why is that? The TiledMap loaders are in the maps package.

@dermetfan
Copy link
Contributor Author

@badlogic, moving it there won't be enough. It also needs a non-arg constructor like the old PolygonRegionLoader had.
@MobiDevelop Why do we even have an assets.loaders package then? One system should be enough. Putting them all in their according packages or keeping them in one place sounds better to me.

@MobiDevelop
Copy link
Member

To me, it depends on whether the loader is strictly for use with the AssetManager. The map loaders, and this loader are not, so they belong in the respective package (in my opinion).

@dermetfan
Copy link
Contributor Author

Fair enough. That's also a reasonable guideline.

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.

None yet

3 participants