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

Minor bugfix and typo correction #134

Merged
merged 15 commits into from Jun 19, 2018

Conversation

Projects
None yet
2 participants
@TheRamenChef
Copy link
Collaborator

TheRamenChef commented Jun 10, 2018

Eclipse won't show me a call hierarchy for the fields I renamed, so I kept the old name but deprecated it.

TheRamenChef added some commits Jun 10, 2018

Apply previous change to getEntities
This was causing a bug where entities in the new render types were not updating.
Handle interrupts properly
There is no need to log the interrupt as an error. The `terminate` method would also be better implemented using the `interrupt` method.
Merge pull request #1 from gurkenlabs/master
Update to base fork; see if that fixes any of the problems.
@TheRamenChef

This comment has been minimized.

Copy link
Collaborator

TheRamenChef commented Jun 19, 2018

I’m not sure why the Travis CI build is failing, or what I can do to fix it. It says "illegal forward reference" but there definitely isn’t an error there.

@steffen-wilke steffen-wilke self-requested a review Jun 19, 2018

@steffen-wilke

This comment has been minimized.

Copy link
Collaborator

steffen-wilke commented Jun 19, 2018

Many thanks for your contribution to the LITIengine!

One general thing I'd like to mention:
Currently, we're still at a stage of development, where we try to keep things clean rather than backward compatible. This will change from the first beta version onward. As long as we're still in the alpha we decided not to use any @Deprecated annotations to at least keep the engine clean until its first beta release v0.5.0-beta. So for now: please just rename methods and constant fields and we'll mention this in the next changelog.

Regarding the singleton change:
For the sake of a simpler API, I think we can just drop the IMapLoader interface entirely. This way, we can make the loadMap(...) method static and consider renaming the TmxMapLoader to MapLoader. The LITIengine currently doesn't support any other map formats and if we decide to implement support for other formats in the future, we'll have to rethink a lot of the current implementations anyway.
PR API state:

IMap map = TmxMapLoader.INSTANCE.loadMap("some-map.tmx");

target API state:

IMap map = MapLoader.load("some-map.tmx");

Regarding the failing build:
I'll clone your fork later today and see what I can find. On what OS are you developing on?

steffen-wilke added some commits Jun 19, 2018

Fix build for PR #134
Remove deprecated elements.
Remove unnecessary imports.
Clean up usage of brackets.
Change the TmxMapLoader to a static MapLoader class since there is no other supported map format.
@steffen-wilke
Copy link
Collaborator

steffen-wilke left a comment

I've fixed the few things that I would have liked to be changed myself.

@steffen-wilke

This comment has been minimized.

Copy link
Collaborator

steffen-wilke commented Jun 19, 2018

Besides minor syntactic issues that caused the original build failure, it turned out that our build script was not prepared for pull requests from other forks ...

@steffen-wilke steffen-wilke reopened this Jun 19, 2018

@steffen-wilke steffen-wilke merged commit 4776340 into gurkenlabs:master Jun 19, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment