juniper-wright / UtopianEngine

The Simple Text Adventure Game Engine
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

split up UE.java, implemented load/save, ue.ini-settings #85

Closed madpew closed 10 years ago

madpew commented 10 years ago

I moved some parts of UtopianEngine out of the (huge) main-file for better code maintainability. I decided to create Game (for all data related to the current game), Gamestate (for all data related to the progress/playerstate) and BuiltIn (to store all script-functions).

Implemented loadstate/savestate-commands and added it to Egerfortia. ( maybe solves issue #43 ? )

Added a Settings-class that reads/writes ue.ini at startup. Currently the only settings is linelength. ( see issue #52 )

Feedback welcome.

juniper-wright commented 10 years ago

Some issues:

  1. The gamestate class sounds like a good idea on paper, but considering that each Room's roomstate is a state variable, that could get messy.
  2. Pursuant to 1, your loadState and saveState functions don't keep track of the roomstates, which are extremely important for saving the game.
  3. in loadState, you redefine the inventory array, which is... odd, since the length of the array should never change unless you load a new game. As is, making a savestate, modifying it, then loading it, could crash the game (via ArrayOutOfBoundsException) when it tries to find items later in that array.

And some comments:

  1. I love the concept of breaking the UtopianEngine class apart into discrete classes; 1200 lines was becoming quite a headache. I'm just not sure this was the best way to do it.
  2. What was your thinking behind the name "BuiltIn" for the class that handles UtopiaScript commands?
madpew commented 10 years ago

ad 1&2: Oh wow, thanks for pointing that out. I really didn't think about that. Should I try to get the roomstates into the Gamestate or scrap the idea alltogether? ad 3: Indeed, it's totally stupid to recreate the array there.

ad Comment 1&2: I started splitting the code by topic, with the intention to make it easier to find where new code should belongs. Wanna change something about games > Game, Persistence Data > Gamestate. "BuiltIn" was just chosen to represent that those functions are "built in" the engine for the script to use. Could also be "Command" or something similar. I'd like to have those Scriptcommands seperated because they shouldn't be altered (would break existing games).

I'll try to modify the PR accordingly.

madpew commented 10 years ago

Added: Gamestate.load/save now also load/save the roomstates. Fixed: No more resizing the inventory on loadstate Added: another way to load gamestates. (intended for use with Sequels).

When using Gamestate.load with isSequel set to true, it ignores the gamename-check and only loads the inventory, score and position. For this to work correctly, the sequelgame needs to be careful with the items.

feedback and suggestions for renaming the BuiltIn-class welcome

juniper-wright commented 10 years ago

I think the Gamestate idea should be scrapped, because it honestly makes more sense from an OOP perspective to have the roomstate in the Room class. You could just move all of the Gamestate functionality into the Game class.

Personally, I'd name the BuiltIn class to "UtopiaScriptHandler" or something similar.

madpew commented 10 years ago

Ok I'll merge Gamestate into Game and rename the Builtins

juniper-wright commented 10 years ago

Not a comment on your modifications, but after having the concept of UtopiaScript bouncing around in my head for the longest time, it's really nice to finally see its name actually displayed in the code. I'm really starting to enjoy your splitting the UtopianEngine class into chunks.