search

Tomash667 / carpg

Combination of action rpg with roguelike.
https://carpg.pl
MIT License
16 stars 15 forks source link

Items now have component-based design #441

Closed BottledByte closed 4 years ago

BottledByte commented 4 years ago

Items now have component-based design instead of inheritance based one.

Refactor Item class and its associates so that it is now allowed to have multiple ItemProperties on a single Item object. This allows that Items properties can be checked and retrieved as needed, allowing to create new interesting behaviours, (such as consumable weapons, inscripted shields, etc.) in the future.

Additional context: This is quite a substantial change. I tried to fix obvious errors, but a lot of code currently is not designed for component-based system and some hacks were needed to keep existing mechanics working. Future review and redesign of associated mechanics recommended.

Feel free to reject this PR. It is just an attempt to make the game's code more generic and flexible for the future.

Leinnan commented 4 years ago

Great work!

BottledByte commented 4 years ago

I have done some changes based on gathered feedback and made code a bit cleaner. Properties are now stored in separate maps and Item only contains bit flags that mark if Item has certain property (which then can be retrieved from storage map).

As of @Tomash667 's concerns about this change:

performance - keeping all this pointers and every access to item require extra level of indirection.

There are far bigger performance hits than pointer accesses. Not to mention that Item access and processing should not happen in every game tick, but rather when needed. Also, modern computers are powerful enough to handle "a few pointers". For these reasons, I deem that the change is negligible from performance point-of-view.

component based approach doesn't fit static game data that is loaded at startup.

It was very easy to switch from inheritance to composition at startup data loading. There are far more problematic parts of code affected by new component design than ItemLoader. Also, content loading will eventually need a change, and this change only makes it easier.

all this extensibility probably won't be used for anything that can't be done with simpler approach.

Simpler approach. Throughout the game's code, this simpler approach you speak of can be seen. 2000+ lines spanning switches and nested ifs, code duplicity, occasional hacks. There is a long list of things and mechanics that you would like to implement. With current state of things, this will result in even more mess than already present. This change tries to ease development of new mechanics by making things more flexible.

There is only one objection against this change that I recognize as valid (and it was not mentioned by you). This change will definitely introduce new bugs. I do not know game's code well enough to predict them and this change affects a lot of code.

As I wrote in my opening comment: "Feel free to reject this PR. It is just an attempt to make the game's code more generic and flexible for the future."