Add option to toggle monthly artifact change in black market #341
Conversation
|
Well, this is a feature borrowed from H5 (similiar to dwellings accumulate creatures). |
dydzio0614
force-pushed
the
blackmarketfix2
branch
from
8c6fab7
to
5562eb6
Compare
|
As Warmonger noted this kind of feature of VCMI. |
|
And then it's need to be added on Wiki too: |
dydzio0614
force-pushed
the
blackmarketfix2
branch
from
5562eb6
to
5d74c3e
Compare
lib/CModHandler.h
Outdated
|
|
||
| template <typename Handler> void serialize(Handler &h, const int version) | ||
| { | ||
| h & data & CREEP_SIZE & WEEKLY_GROWTH & NEUTRAL_STACK_EXP & MAX_BUILDING_PER_TURN; | ||
| h & DWELLINGS_ACCUMULATE_CREATURES & ALL_CREATURES_GET_DOUBLE_MONTHS & | ||
| MAX_HEROES_AVAILABLE_PER_PLAYER & MAX_HEROES_ON_MAP_PER_PLAYER; | ||
| if(version >= 756) | ||
| { |
There was a problem hiding this comment.
If you didn't do that yourself please disable this automatic curly brackets removal. In many cases it's makes sense to avoid or remove them, but serialization compatibility code is not one of those places.
There was a problem hiding this comment.
Oh, I removed these on purpose as they stick to my code where I used "no bracket style" and changing that does not ruin PR readability
There was a problem hiding this comment.
I think we should always use brackets in serialization code, but it's all about it.
There was a problem hiding this comment.
So should I change to brackets?
There was a problem hiding this comment.
Yes. I'll probably have to write something about that on wiki.
I not sure if we have any article about compatibility code yet.
lib/mapObjects/CGMarket.cpp
Outdated
| saa.id = id.getNum(); | ||
| cb->pickAllowedArtsSet(saa.arts, rand); | ||
| cb->sendAndApply(&saa); | ||
| if(VLC->modh->settings.BLACK_MARKET_MONTHLY_ARTIFACTS_CHANGE && cb->getDate(Date::DAY_OF_MONTH) == 1) //new month and feature enabled |
There was a problem hiding this comment.
I would suggest to use dedicated if instead:
if(!VLC->modh->settings.BLACK_MARKET_MONTHLY_ARTIFACTS_CHANGE)
return;
So it's easier to understand is some very special hardcoded option and not just one of parameters.
There was a problem hiding this comment.
Okay, though apart from this pull request - isn't it better practice to use "if" for main code logic and leave the rest for automatic return instead of explicitly quitting from function at beginning?
There was a problem hiding this comment.
I personally don't have any strong opinion on that topic, but I think it's depend on what is your code doing. If it's some kind of rules checking or input validation there nothing wrong with multiple checks instead of large one.
dydzio0614
force-pushed
the
blackmarketfix2
branch
from
5d74c3e
to
edb1856
Compare
|
Should be merged after #323. |
dydzio0614
changed the title
dydzio0614
changed the title
dydzio0614
changed the title
EDIT: Implemented as "hardcoded feature".
Fixes mantis #2714.