Reworking the control flow for my tactical role-playing game

[Liam McGillivray]

On Saturday, 23 March 2024 at 04:32:29 UTC, harakim wrote:
> You should engineer the system in a way that makes sense to 
> you. The currency of finishing programs is motivation and that 
> comes from success and believing you will succeed. If you're 
> implementing XYZ pattern from someone else, if you don't get it 
> then you will get unmotivated. I have seen some pretty horrible 
> programs make it to all the way done and have almost never seen 
> someone's first run at a problem be both pretty and get 
> finished. Don't beat yourself up about keeping it clean. If you 
> feel like it or it's getting out of control where you can't 
> understand it, then you should go look for a solution. The 
> motivation is in moving forward, though. This is probably the 
> piece of advice I wish I had in the beginning! You can always 
> refactor later when it's done.

Alright. I suppose this is largely what I'm doing.

Thank you very much for looking through my code and making the 
effort to understand it. So far you are the first person to 
comment on my code, beyond the little excerpts I've pasted 
directly onto this forum.

> Move to the tick system. You make a loop and update each object 
> based on the time that has passed. If your tick is 1/60th of a 
> second then you make a loop. In the loop, you update the map 
> state knowing 1/60th of a second has passed. Then you update 
> all the items and move them or deplete them or whatever however 
> much would happen in 1/60th of a second. Then you update all of 
> the units. Then you can update the factions or check win 
> conditions. At the end of the loop, you draw everything.

When you said "tick system", I thought you meant that it would be 
asynchronous to the framerate, but then you say "At the end of 
the loop, you draw everything". Right now the function that moves 
units across the screen moves them each frame a distance 
proportional to the duration of the previous frame. I was 
thinking that a tick system would go hand-in-hand with making 
animations happen in a separate thread, but it sounds like you're 
talking about the same thread. Are you suggesting a fixed 
framerate?

Because this is a turn-based game, I don't need to update the 
factions and win conditions every frame, but at most every time a 
unit makes a move or when a new turn starts.

> Your verifyEverything method is awesome. I call that strategy 
> fail-fast. It means you fail right away when you have a chance 
> to identify what went wrong.

That's nice. I thought it might be an amateurish solution to 
something that better programmers would do differently, similar 
to how I use `writeln` to print variables for debugging. It looks 
like I don't yet have any calls to this function, so perhaps I 
should add one under the debug configuration.

> Construct the unit and then call map.setOccupant(unit) after 
> the unit is constructed. I would not do anything complicated in 
> a constructor. It's also generally frowned upon to pass a 
> reference to an object to anything before the constructor 
> completes. Most of the changes I mention are things to think 
> about, but this specifically is something you ought to change.

I didn't immediately understand what you were saying, but then I 
looked at what line `unit.d:44` was at the time you wrote this.
```
map.getTile(xlocation, ylocation).setOccupant(this);
```
This is during the Unit's constructor, where it gives the `Tile` 
object a reference to itself. What exactly is wrong with this? 
Can memory addresses change when a constructor completes? I 
assumed that objects come into existence at the beginning of the 
constructor function.

Anyway, I can change this by calling `Unit.setLocation` after 
creating a new `Unit` object. That's unless if there's a 
particular reason why you think I should make this a function of 
the `Map` object.

I suppose the current system for setting up objects from a 
`JSONValue` is messy because much of the data applies to the base 
`Tile` & `Unit` objects, but then some of it needs to be in 
`VisibleTile` & `VisibleUnit` because it has sprite information.

> I would also remove the unit from the map and then delete the 
> unit rather than removing the unit from within the map class.

Sure. I'll change that. Even when I wrote the Unit destructor I 
was thinking that perhaps this was a bad way to implement this. I 
suppose I should just call the `Map.deleteUnit` whenever a unit 
should be deleted, right? I was also thinking of replacing 
`Map.deleteUnit` with a function that removes all null references 
from `this.allUnits` which can be called after destroying a unit, 
but unless if I need to hyper-optimize, that probably won't be 
any better than the function I have.

> Another reason I would switch that line is that it's best to 
> avoid circular dependencies where you can. It will make it hard 
> to reason about either in isolation. It relates to that line 
> because your map has units in it and your unit takes Map in the 
> constructor. That is a red flag that you are too coupled. That 
> concept is not a rule but just something to think about when 
> you get stuck.

I don't quite understand. Are you saying that I shouldn't have 
objects reference each-other, like how a `Unit` object has a 
reference to a `Tile` object called `currentTile`, and that tile 
object has a reference back to the unit called `occupant`? I see 
how bugs may happen if there's ever a case where the references 
aren't mutual, which is the reason for the `verifyEverything` 
function that you praised. But I imagine things getting more 
complicated if I can't determine a unit's current tile just by 
looking at a reference in it. Do you really think it would be 
better if I removed `Unit.currentTile`, and just used 
`Unit.xlocation` & `Unit.ylocation` as parameters for 
`Map.getTile` instead?

> In your game loop, I would keep track of the units separately 
> from the map, if you can. Go through the map and do updates and 
> go through the units and update each one. If the logic is too 
> tied together, don't worry about it for now.

I'm not sure what you mean by "keep track of the units separately 
from the map". Do you mean to not rely on the `Map.allUnits` 
array?

I have considered doing a rewrite of the rendering system, in 
which all the rendering, and possibly input during game are 
handled by a new `Renderer` object, which may have it's own array 
with all the units in it.

> I would break the json loading into separate classes (eg 
> FactionLoader, Unit loader) instead of being included in the 
> map and unit class. I like to have code to intialize my 
> programs separate so I don't have to look at it or think about 
> it or worry about breaking it when working on my main code.

Is this a matter of not having all that JSON-related code 
cluttering the top of the file? Does putting too much of it in 
the objects themselves make them take up more memory? Or does it 
really make bugs less likely?

Before going with the current approach, I intended to have a 
module called `loadData` which would read JSON files and make 
objects out of them, but then I figured it was easier to have the 
JSON data read directly in object constructors so that private 
and protected variables can be set. I've thought about going back 
to this approach, but I haven't because I couldn't think of a 
reason to. If I do, perhaps they should be placed in the same 
modules as the objects being constructed, instead of being placed 
together in the `loadData` module.

> Take side quests when you want to stay motivated, but I would
> stray away from the bigger ones until you have the basic 
> functionality working. It's often the fastest way to get the
> side quest done anyway since you can test it.

Yep. At this very moment I have two feature branches on my local 
copy. I'll have to figure out how to merge them when complete, 
but I did this for motivation-management, so that I don't need to 
finish one feature when I feel like working on another.

> You should probably not do this, but it might give you some 
> ideas for later. What I would do is make a separate thread for 
> managing the UI state and push events to that thread through 
> the mailbox. I have done this once (on my third version of a 
> program) and it was by far the cleanest and something I was 
> proud of. The benefit is you can publish those same events to a 
> log and if something in your UI goes wrong, you can look at the 
> log.

This sounds a little like my idea of the `Renderer` object, in 
which the state of what's on screen would be updated by calling 
it's methods, but having a log of the UI wasn't what I had in 
mind.

That being said, I've thought about the possibility of having a 
log of events that can be used to rewind, replay, or possibly 
save the game. I agree that this should be further down the line. 
Even the ability to save a game is not very high on the priority 
list now. I wasn't thinking of logging *every* UI event though.

> your game engine

Thank you for implying that I wrote a "game engine". That sounds 
like an accomplishment on my part. I'm not really sure which 
parts of my code would be referred to as the "engine" though.