Check loaded mapgen for being used at all. #37425
Conversation
|
#37428 will take care of the houses. I'll let the other creators fix their maps. will this search nested chunks as well? |
|
#37458 Fixed the wrong microlab entry. |
No.Those are stored in a different variable. |
I-am-Erk
added
[C++]
BevapDin
force-pushed
the
lib
branch
2 times, most recently
from
dc55200 to
5867f40
Compare
src/mapgen.cpp
Outdated
| * @p hardcoded_weight Weight for an additional entry. If that entry is chosen, | ||
| * a null pointer is returned. If unsure, just use 0 for it. | ||
| */ | ||
| mapgen_function *pick( const int hardcoded_weight ) const { |
There was a problem hiding this comment.
Shouldn't this function return a constant pointer? The function itself is marked const, but its result points at the class internals which can be changed through it.
| mapgen_function *pick( const int hardcoded_weight ) const { | |
| const mapgen_function *pick( const int hardcoded_weight ) const { |
There was a problem hiding this comment.
Yes, it should. But mapgen_function::generate is not const either, so you would have to cast the pointer anyway in order to call that function. And calling generate is basically the only thing one will do with that pointer.
There was a problem hiding this comment.
generate should probably be marked const either. It's meant to operate on mapgendata and shouldn't mutate its internal state. Anyways, a const function which returns a non-const pointer to its internals is suspicious at the very least.
| /** | ||
| * Calls @ref mapgen_function::setup and sets up the internal weighted list using | ||
| * the **current** value of @ref mapgen_function::weight. This value may have | ||
| * changed since it was first added, so this is needed to recalculate the weighted list. |
There was a problem hiding this comment.
Could you please elaborate on this. Why would weight change after adding to the container? Maybe it should take ownership of the mapgen_function's themselves (instead of possessing mere pointers) and protect its own guts form external interference? The current scheme seems to be rather fragile.
There was a problem hiding this comment.
Why would weight change after adding to the container?
When erase at line 209 is called.
There was a problem hiding this comment.
instead of possessing mere pointer
Nah, pointers are exposed e.g. via load_mapgen_function. If I read the code correctly, this can be used to connect one mapgen instance with multiple overmap terrains (equivalent to giving each overmap terrain a copy of the mapgen). It seems like the mapgen in "lab/lab_trains.json" make use of that.
There was a problem hiding this comment.
I see, thanks. It would be ideal to refrain form int indexes at some point and use persistent iterators instead. Resetting weight to 0 in a temporary container as a sign of an item being erased, and then not being able to operate on indexes after invocation of setup looks, well, a bit hackish.
If you still decide to stick to this approach, it would be nice to issue a debugmsg in case someone tries to call add or erase after setup.
Just get the value and let the JSON system deal with the existence check and the error message. Also throw errors via the JSON system so it creates a nice message with error location.
Basically the whole look up of the mapgen function pointer within the `oter_mapgen` and so on.
This is mostly done to avoid having to expose `oter_mapgen` in the header.
Those variables are already declared in a header or not even used within this cpp file.
Again: this is to avoid having to expose `oter_mapgen` in the header.
Merges the comments from the header with the comments in the cpp file for the very same objects.
Those instances can have their own members and stuff.
Instead of having a separate map, this reuses the already existing entries in `oter_mapgen`.
… into the vector itself. This avoids the repeated look up within the vector and it avoids a potential error situation: the index taken from `weight_` could be invalid.
We already have this class, so we should make use of it.
…stency Note: the function name is chosen for consistency (no pun), our other consistency checking function are named that way.
No need to expose them. The last direct access to the vector had to be wrapped within a new member function.
And move all the functions that deal with it into that class.
…ype (and what does the "f" even stand for?)
This will find mapgen instances that were loaded from JSON, but have no usage within the game (e.g. because the belong to undefined overmap terrain). Register some mapgen ids that are only used within the C++ code as used
…ctory` Don't expose them at all. All access to them is done internally.
This terrain should never appears and is therefor never generated. Removing it here may free up some memory.
BevapDin
changed the title
Adds the default value to all calls to it that did not supply a value.
This feature is not available anymore. It was added by 04e45ea without any explanation.
SUMMARY: NoneThis refactors the
oter_mapgenvariable a bit. It merges theoter_mapgen_weightsinto it, so we do not have to look up the entry in two maps.It uses
weighted_int_listinstead of keeping yet another implementation of the same algorithm.Finally it adds checks for the loaded mapgen instances to be used. As it turns out, some are not used at all:
The game will now issue debug messages for each of those mapgen instances, but it will continue normally.Do not merge right now (therefor I added the WIP tag). Fix the usage of the mentioned mapgen instances first: either add a usage (e.g. by adding a matching overmap terrain) or remove the mapgen instance.