Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tavern rumors and Thieves Guild fixes #145

Merged
merged 11 commits into from Dec 5, 2015
Merged

Conversation

@ArseniyShestakov
Copy link
Member

ArseniyShestakov commented Nov 30, 2015

Work in progress on all 3 kind of rumors:

  • Random rumors.
  • Stats rumors that depend on thieves guild info + grail.
  • Custom rumors from map.
Original game used "Rumors" and not "Gossips" and we already using rumor in CMap.
Actually information about this week rumor should be stored in gamestate and updated weekly
@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Nov 30, 2015

There is very little details on rumors mechanic and zero reports on bug tracker so here is some info:

  • Game has 3 types kind of rumors as posted above.
  • Rumor changed weekly and shared between all players.
  • Rumors of same type may go in row. Likely there is % change that one of type appear.
  • Same rumor is never repeated twice in a row and this is why we must keep one previous rumor for each type. That is valid even if there was different type of rumor in between (e.g stats rumor -> map rumor -> stats rumor and last one have to be different stats rumor).
  • When multiple players occupy 1st position in thieves guild rating the first one appear in rumor (e.g red).
    I feel this one is a bit unfair so I'll just choose random player instead (from all who's on 1st position).
Multiple rumors of same type can go in a row, but not identical rumors.
This also fix income ranking in Thieves' Guild
@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 1, 2015

Income code is really bad for now, but I have some ideas to refactor StatsHLP and reuse it for both client (Kingdom Overview) and server (instead of current income calculation on new turn).

@DjWarmonger

This comment has been minimized.

Copy link
Member

DjWarmonger commented on a62ee65 Dec 1, 2015

This can't be static. Info should only be given via callback if player is allowed to know it. This is general cheat-proof design and encapsulation.

@edeksumo

This comment has been minimized.

Copy link
Contributor

edeksumo commented Dec 1, 2015

I'm to start work on this -_- but you made more than me, so i will stop. But you can add rumors for mod just like i want to do in my wip.

@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 1, 2015

@edeksumo
We can merge code if you fixed something else and you can continue work on it. It's clearly not something I want to work on exclusively while both rumors and Thieves Guild require more time if we want to authentic H3 behavior.

Sent you PM on forums with my skype contact if you agree to talk.

@DjWarmonger
BTW also wish I had your contact that isn't just forum/github. :-)

@Alucard648

This comment has been minimized.

Copy link

Alucard648 commented Dec 1, 2015

I am hoping there will be an option to get new rumor for gold, like in Heroes 5.

@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 1, 2015

@Alucard648
There is already rumor in H3 for gold income:

%s earns the most gold.

Any example of what rumors you mean?

@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 1, 2015

Just in case I'm not planning any modding support there for now, but I'll need to take it into account to not break rumor serialization formation in future. E.g currently I only store rumor type, rumor ID and additional ID (terrain for grail or player).

Though I suppose some modability may require more complex struct.

@Alucard648

This comment has been minimized.

Copy link

Alucard648 commented Dec 1, 2015

I mean buy new rumor in tavern for 100 gold, instead of waiting another week.

@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 1, 2015

Ah, okay. Sadly something like that would require a bit different implementation. In H3 rumors are always shared between all players while mechanics like that may require to make rumors unique per players.

It's not hard at all to implement, but then we need to store rumors differently and actually avoid showing duplicates, etc.

@IvanSavenko

This comment has been minimized.

Copy link
Member

IvanSavenko commented Dec 3, 2015

Income code is really bad for now, but I have some ideas to refactor StatsHLP and reuse it for both client (Kingdom Overview) and server (instead of current income calculation on new turn).

This looks like one more case where "object roles"/mixins may work. Each object that provides daily income implements "IDailyIncomeRole" and to get all your daily income all you need is to sum daily income of all owned objects with daily income role.

So I'd suggest to keep it as it for now since I've started looking into this - no need to duplicate effords.

@ArseniyShestakov

This comment has been minimized.

Copy link
Member Author

ArseniyShestakov commented Dec 3, 2015

Okay then I won't touch it for now and just merge it as it's now with other branches.

@ArseniyShestakov ArseniyShestakov force-pushed the feature/tavernRumors branch from 71eaa5a to 3b3c494 Dec 4, 2015
@ArseniyShestakov ArseniyShestakov changed the title [WIP] Tavern rumors Tavern rumors and Thieves Guild fixes Dec 4, 2015
We also now check not number of towns, but only towns that has tavern built. Also according to original mechanics all taverns always display information based on your number of taverns and not number of taverns of object owner.
{
return "GOSSIP TEST";
std::string text = "", extraText = "";
if(gs->rumor.type == RumorState::RUMOR_NONE) // (version < 755 backward compatability

This comment has been minimized.

Copy link
@IvanSavenko

IvanSavenko Dec 4, 2015

Member

Just a thought - what about marking all places for backwards compatibiltiy in some consistent format? Or even static_assert'ing that this version is not less than minimal supported?

This would allow us to remove all compatibility code once save version is increased instead of accumulating it over releases.

void CGameState::updateRumor()
{
static std::vector<RumorState::ERumorType> rumorTypes = {RumorState::RUMOR_MAP, RumorState::RUMOR_STATS, RumorState::RUMOR_RAND, RumorState::RUMOR_RAND};
static std::vector<int> statsRumorTypes = {208, 209, 210, 211, 212};

This comment has been minimized.

Copy link
@IvanSavenko

IvanSavenko Dec 4, 2015

Member

Why are these magic numbers needed? What about one more enum? If you need these constants then just define enum with fixed values ( RUMOR_GRAIL = 212 )

Ah, found how they are used. Still I'd prefer constants or enum's over magic numbers.

if(obj && obj->tempOwner == ps->color)
ownedObjects.push_back(obj);
}
/// This is code from CPlayerSpecificInfoCallback::getMyObjects

This comment has been minimized.

Copy link
@IvanSavenko

IvanSavenko Dec 4, 2015

Member

(unrelated to pull)
We should just store all player-owned objects in a list instead of current object-specific lists of heroes, towns, dwellings.

ArseniyShestakov added a commit that referenced this pull request Dec 5, 2015
Tavern rumors and Thieves Guild fixes
@ArseniyShestakov ArseniyShestakov merged commit 360ebcc into develop Dec 5, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ArseniyShestakov ArseniyShestakov deleted the feature/tavernRumors branch Dec 5, 2015
ERumorType type;
std::map<ERumorType, std::pair<int, int>> last;

RumorState(){type = TYPE_NONE; last = {};};

This comment has been minimized.

Copy link
@edeksumo

edeksumo Dec 17, 2015

Contributor

Before I can't test it because I have't got any time, so now when I test it, this one provides me an error: operator =' is ambiguous

This comment has been minimized.

Copy link
@DjWarmonger

DjWarmonger Dec 17, 2015

Member

Confirmed. This does not compile in Visual and I don't see how it could.

This comment has been minimized.

Copy link
@alexvins

alexvins Dec 17, 2015

Member
RumorState(): type(TYPE_NONE), last(){};

Is this works?

@edeksumo

This comment has been minimized.

Copy link
Contributor

edeksumo commented Dec 17, 2015

Probably not: Error 5 error C2440: 'return' : cannot convert from 'CGTownInstance *' to 'CGObjectInstance *' in another file: CommonConstructors.h

@IvanSavenko

This comment has been minimized.

Copy link
Member

IvanSavenko commented Dec 17, 2015

Yeah, CommonConstructor.h needs some includes.
You need to include CBank.h, CGTownInstance.h and CGHeroInstance.h

@edeksumo

This comment has been minimized.

Copy link
Contributor

edeksumo commented Dec 18, 2015

It's work, if nobody fix it tomorow I will make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.