search

LambdaHack / LambdaHack

Haskell game engine library for roguelike dungeon crawlers; please offer feedback, e.g., after trying out the sample game with the web frontend at
https://lambdahack.github.io
BSD 3-Clause "New" or "Revised" License
621 stars 55 forks source link

Review of commit review #3

Closed Mikolaj closed 13 years ago

Mikolaj commented 13 years ago

Here I will gradually add pieces of the the promised review of your modifications/reversions of my commits. Thank you again for taking time to look at the commits in detail.

I have essentially reverted the last change that removes the "open door" command, because I disagree with it. I'm happy to add auto-open as a configuration option later, but I don't like it to be the default for everyone.

I can live with it, it's a single small detail, but in my experience:

  1. KISS. Options are bad, the less options the better, except debug and experimental options and unintrusive UI options like skip splash screen or macros. This is, among others, because options confuse players, bug reports are less useful if different options can be used, code for both values of a boolean option has to be maintained (notice that 'close' is quite a bit simpler that 'openclose') and tested (usually the option not used by the maintainer bit-rots fast or becomes mildly incompatible with new features).
  2. Keys are bad, the less keys are used by the game, the better. The same with menu items.
  3. The 'bump into things to affect them' metaphor works well even in very complex games and bumping into a door to see no effect weakens the metaphor. I also offers faster play, less keystrokes, which matters a lot after a few hours of play.

-- TODO: Turn running into a standalone handler. Make it clearer and -- more configurable how running behaves, and when it stops. This is -- nothing the game should force on a player, but that a player should -- rather be able to configure.

In a game engine, running behaviour should indeed be easily configurable. But in concrete games build upon the engine, usually only one setting is optimal (diagonal moves, if there's a time limit and no instant death threats (e.g. paralization); avoiding diagonal moves otherwise), so a player-configurable option falls under the "bad option" category above. OTOH, as a debug or experiment option, it's a very good idea and I should have done it in a configurable way, instead of just changing the behaviour according to how I imagine LambdaHack will evolve (no hurry exploring and pretty dangerous attacks).

I am reverting a premature optimisation in the original patch. However, I keep the changed semantics of running which is a clear improvement over the original. However, I'd like running to be more configurable in the future.

Agreed on all accounts.

Revert "minor tweaks ported from branch branching_degree" I revert this change because I disagree with nearly all aspects of it. For example, let's try to keep functions as total as possible.

I'm happy to learn and adhere to your stylistic preferences (I'm a Haskell newbie, after all), but I have to explain a few things, because my "minor tweaks" were not all about style, but about experience with modifying the code, after a failed experiment on a different branch. There is no point in you reading the branch nor in me explaining the details, so just a few concrete points about the code before I changed it in my commit:

  1. digRoom sometimes creates isolated, 1-tile rooms. They look out of place and very rarely items and monsters (and stairs?) can be trapped in them. The rooms are useless, if not isolated, because they just get overwritten.
  2. L.nub documents that sometimes the corridor can have less than 3 sections and prevents confusion if, in the future, the dungeon keeps a local or global counter of corridor turns or intersections (for AI or to describe floor in an intersections as "worn", etc.).
  3. digCorridor in three cases preserves items in a tile, but in one case loses them. The "undefined" value (in another commit, here it's "dummy") documents that only the locations from the newly created list are important, further ensuring that items from the 2 merged tiles will not be mixed up. Granted, there are no items in tiles at this stage of dungeon generation right now, but...

Revert "the space key no longer skips turn" Reverting only to comment the line. This should be configurable.

Agreed. Macros are "good options". I've only removed resting on space, because confirmations where done with space or enter, so space was often pressed by accident. If all confirmations are done with y/n, the space can stay or replace dot. OTOH, we shouldn't take 2 keys (3 with keypad '5') for the same task without a good reason. Let the player take as much of them as he want via macros and let's leave him lots of unused keys to work with.

Revert "hardened the highscores file handling code" I fail to see why not addressing other exceptions is a bad thing, so I'm reverting the patch.

I may be wrong, but I think if you have bad permissions on the file or play from / or from live CD or broken NFS, you will never learn, until you become suspicious about the score table not growing longer with time. The patch let's you see it ASAP via an exception displayed on command line.

scan (tr loc) lmap 1 (0,1)) -- was: scan (tr loc) lmap 0 (0,1); TODO: figure out what difference this makes

I'm not 100% sure, but I seem to remember I tested it and came to the following conclusion:

scan (tr loc) lmap 1 (0,1)) -- with 0 in place of the first 1, if the central square is not transparent (e.g. if it's a curtain), nothing else is visible

That's it. The PassiveHandler removal is surely for the best. Agreed about PLAYING.markdown. I've done a diff between my and your changed version of all the commits and it seems you didn't lose even a single line while juggling the patches and producing the git art. Thanks again for the review.

Mikolaj commented 13 years ago

That's it, final edit, done.

P.S. I like the new Turn.hs, using Action.hs. I agree with all your comments in Turn.hs (though I will need to see how the even queue is used to understand the idea).About score for items: in the long run let's not give score points for items other than gold. For now, most items are worthless, hence the score, to make gathering them engaging.

Mikolaj commented 13 years ago

These remarks are already discussed/obsolete/used either in LambdaHack or Allure, so I'm OK to close this issue. Thank you again for the old and for the new review; most of the suggestions were very fruitful. whether I agreed with them or not.