Closed koosemose closed 5 years ago
I would just merge it, it is extremely simple and really just breaks up a function :).
But it gets my 👍
== Edit ==
Once you are done of course. And in fact I have a few questions.
The loading here seems really weird, why do we need to set the three ints for this to work? Why is the World constructor creating a character? What happens to the character that is created? That happens to all the already initialized fields when we load the json?
These are just the questions I get from a quick look at the code.
Looking over this again (just giving it a deeper look), was just about to make comment on removing the extra function but you already did that!
Also since SetupWorld isn't called in the constructor when we are creating an empty world it won't setup any of the required stuff?
The three ints are the world's proper size, which the world generator needs to know.
The character is created in the world constructor because we want to have a character available immediately, and we either don't have a better place to do it from or at least no one has bothered to find a better place.
The character continues to exist.
The already initialized fields should either be ones that aren't overwritten (with the exception of the two that I overlooked in the constructor itself, or the 3 world size ints, which are needed by the world generator, and get set back to the proper values after the load happens. Which after further consideration I've altered to just pass those values directly to the world generator.
And generating an empty world just loads a dummy empty starting area, so it still ends up calling SetupWorld, just from a different place.
Also, this is exactly why I left this open rather than merging it, I know I was tired when I first submitted it (still am), so am likely to have made mistakes, particularly ones related to only looking at the procedures a step at a time rather than the whole picture.
Also @NogginBops , if you see any other things that are created twice, let me know specifically, so I can either resolve them, or if for some odd reason they need to be that way, explain why and hopefully come to a less odd solution.
The only thing of note that may not be obvious if you miss a step in code is things like the tiles array or similar things that rely on the size of the world, rather than implementing some kludge involving every buildable that cares about tile position being able to have offsets to position them correctly or whatever, I "resize" the structures afterwards (which in most cases, perhaps all, is creating a copy of the old structure and copying the data from the older version in appropriately offset positions)
This splits SetupWorld into two parts, the first part required before generating a new world can work properly.
The one part I am dissatisfied about is that PreinitializeWorld is called twice, as it also needs to happen before a world is loaded from file. I do however consider this relatively minor as Preinitialize only sets 3 int values. If this is considered a significant issue, I see two possible solutions, the first being to have GenerateWorld passed the world dimensions (the values it needs set beforehand as it currently functions), and setting them directly. Or find somewhere outside of the ReadJson function (which is where Preinitialize is called the second time when generating a world) to call PreinitializeWorld (If this is the preferred option, I couldn't find anywhere reasonable to do so after a cursory look, and am open to suggestions, in fact will need them).
Final comment, I am open to suggestions on function names (either to PreinitializeWorld, or to change SetupWorld to a similar form as Preinitialize, perhaps just InitializeWorld, as some may find it slightly misleading in what it does).
Edit: This is now in a state that I would consider ready, barring the concerns mentioned previously.