search

FIUS / ICGE

Introduction Course Game Engine for the FIUS Java Introduction Course
MIT License
3 stars 2 forks source link

treewide: Replace `EntityType` by `EntityState` #37

Closed haslersn closed 6 years ago

haslersn commented 6 years ago

This is a rather big change and needs some review. The API changes however are minimal.

buehlefs commented 6 years ago

This would lead to a rather big time waste getting everything in our exercise sheets and Task/Solution classes up to date for minimal benefit. Is there a reason to merge this request before monday or can it wait until after the java introduction course is finished?

haslersn commented 6 years ago

The places where the tasks and solutions have to be changed are extremely easy to find via the compile errors. It took me 2 minutes to do all of the needed changes locally.

With this change, students can add Entity classes without modifying the ICGE.

buehlefs commented 6 years ago

That is a good point for Luigi. However the rendering and collisions still use hardcoded class checks. So entityState at least needs a method like isSolid() and one like getSpriteSet() or getSpriteId()

haslersn commented 6 years ago

Such stuff can be added in the future no matter whether we merge this PR or not.

buehlefs commented 6 years ago

But then we have exactly the same situation as with EntityState. We have to implement both the EntityState and the EntityClass for luigi for us to be able to show the correct sprites for luigi. We can do the same with the current EntityType and already have a finished pr for that. As EntityType and EntityState rely on different concepts to work they look and feel quite different. The developers of the tasks would also need time to adapt to this change.

haslersn commented 6 years ago

In the tasks there was conceptually a single thing that changed: Instead of EntityType.WALL, use new WallState(). (Same for other EntityTypes.)

The finished PR for Luigi just adds Luigi but the students cannot write Luigi themselves. If you'd remove the Luigi class from the PR, it wouldn't compile, since the EntityType.LUIGI in the ICGE uses Luigi::new. The only way to introduce new Entitys from JVK-2018 would be to prepare the corresponding EntityType in the ICGE and also create the Entity's class in JVK-2018 including a constructor.

buehlefs commented 6 years ago

We could also change the EntityType enum to include a createEntity() method that does not work for luigi. As this method would only be called if luigi is set via a territory editor which we never do this would have nearly the same effects as this pr with minimal changes. Since the course starts on monday i would prefer minimal changes...

haslersn commented 6 years ago

Is there any problem with the PR besides that the tasks have to use new WallState() instead of EntityType.WALL?

Besides the possibility to add arbitrary new Entity classes, this PR also has the useful effect that entity state is never lost when using setTerritory().

I updated the PR to use our newest master such that it merges fine.

Also, here's the patch for the current master of FIUS/JVK-2018@d014405d79f2ef1f7fd23829703eeeb2ad072404 which includes the necessary pom.xml change, such that everybody can try it out:

FIUS-ICGE-pull-37.txt