This is really make me think why is that done this way and if this need to be changed as I need denyTransit option to be available for absolutely all visitable objects. Or possible I misunderstand you.
About 'doMoveHero", I’m really wish this code was more or less the same as VCAI code…
Also I think back to idea where the full path submitted to server-side before movement start because this way we’ll able to validate it once it’s received and this will decrease complexity code on server-side a lot. Our past discussion on github: #90
So thanks to @AVS I improved “doMoveHero” code to state I like it and this also fixed last desync. Though I’m still planning to reimplement movement anyway, but seriously want to merge what already done first.
Also I made decision to disable one-way monoliths for now.
There is following reasons behind it (from my commit):
[ul]]Cheating. Currently to check if there one exit or more client need access to channels data that include not yet seen monolith id’s. In same time I want to have two different options: one for one-way channels with one known exit and other for channels with multiple exits./:m]
]Accidental collisions and transit. I don’t like idea to enable transit through one way entrances as it’s will break some unique mechanics on maps. Without transit player may go into them accidentally which is really bad./:m]
]Such feature have to be toggable and I’m need to find out how to implement it within config/UI. E.g I had idea to implement “Advanced options” in launcher for long time./:m]
]AI clearly need a lot of special handling for them because otherwise it’s will get in trouble on some maps./:m][/ul]
Once I solve these issues I’ll re-enabled them, but for now nothing of it going into this first pull request.
Why do you need this option for every object? What exactly is it for?
If you want to check how object visiting happens (e.g. hero must stand on visitable tile or access it from neighbor tile) then this is already present - blockVisit property IIRC.
If object is visitable and allow “transit” pathfinder let hero build path over it. So hero can move over any visitable tiles and for example don’t visit teleporters on his way automatically. Hero also automatically capture any objects that can be captured within transit.
Though there is several objects that shouldn’t support transit: events, whirlpools and one-way monoliths. So option to deny transit is needed.
This is a bit more complex than that.
What if mine is guarded?
What if hero has no taverns? He will be unable to see armies in towns/garrisons
What if object has some negative effects from visits, e.g. Witch Hut with poor skill?
Even objects like chests can be tricky - what if I accidentally will pick chest with secondary hero while I wanted to give exp. to primary hero?
So I’m not sure if this is a good idea. And besides - this is clearly part of object logic: can I pass through it without side effects? It should be part of CGObjectInstance API with type-specific implementation for some/most of game objects, object config is not the best place for it.
I’m never wanted to let hero pass where is not allowed to pass. E.g currently I use “isPassable” check that so for example you can transit via guarded garrisons. So as far as I get you shouldn’t be able to go via guarded mine already.
This is interesting specific. I never know about this problem so will need to test it.
Then we should set “denyTransit” for such objects.
Transit only work for tiles marked as VISITABLE so hero not going to pick anything automatically.
To be fair I’m really hope to see as much options as possible in objects config as it’s best for modding. So I’m not really get why this need to be implemented in code.
There a lot more things need to be moved in config like sounds, reset time, visit limit, etc. Of course it’s talk for different topic.
Really wish there was way to get rid of this CGHeroInstance::convertPosition completely as problems with object/hero positioning are extremely annoying. 
Because this is not static property that is permanent for an object. For example for towns proper test would be something like this:
cantransfer = (ally_owner AND no visiting hero) OR (enemy_owner AND empty garrison AND have_tavern)
Setting deny transit for all object that may not be visitable under some conditions basically means that 90% of map objects must be marked as such.
I’m all for moving constants outside from code into configs but for properties that may change during game scripting is the only real possibility.
Don’t we have something like getVisitablePos that should return proper location for all objects? If not - then we definitely should add this and reduce usage of convertPosition/getVisitableOffset as much as possible.
If problem is setting position (e.g. you want to have hero’s visitable tile at certain position ) then I’d suggest to add inverted function that will perform conversion inside and maybe even make convertPosition private to ensure that all code uses this new function.
Basically we have to check other properties of object anyway and denyTransit only represent if transit is allowed or not while other conditions checked separately. So denyTransit property of object shouldn’t really change in-game at all as it’s only work for what marked as VISITABLE. E.g objects where hero staying are already not visitable.
I think it’s only should be set on objects that may have potentially negative effects and several other objects.
Perhaps just don’t add such uncertain options? In some situations player may want to move through dangers (e.g. battles), sometimes - he may want to visit “bad” objects, etc. While you wish to add option that basically says “nah, I don’t care if you’re OK with visiting this - I said that you won’t go through here”.
Instead I think that we should have:
- Normal pathfinding - it will only go through some locations like owned/empty garrisons & two-way teleports. That’s it - 100% safe options.
- Extended pathfinding, used via some modifier key (ctrl?). It will go thorough anything - neutrals, enemy-held objects, any other objects on the way. It’s purpose would be to show presence of path from A to B so player may check his options, not to build a safe path. Players will be able to use it to find some way into location blocked in one way or another.
But players don’t need to get automatic pathfinder that goes through dangerous objects. AI does, because it is stupid.
Currently there’s SectorMap in VCAI that takes care of guarded tiles and removable objects, however has its limits. For example it cannot find a path through guarded tile which is on the other side of Subterranean Gate, which can lead to infinite loops in some scenarios. It eventually should be merged with updated pathfinder, but this may take time and will be tricky.
However, SectorMap also handles some additional situations, such as buying ship for AI. So there’s no direct translation between pathfinder and AI decisions.
To be fair I don’t get how is this option uncertain. Who care if player go through Garden of Revelation, Learning Stone or Lighthouse without visiting them? 
In same time we can disable it for any objects that give temporary bonuses to hero, negative or positive. E.g any objects that have visit limit so player don’t “cheat” by not visiting them.
PS: And yeah as I said before I wish to implement something more complex in future so player may be able to choose how it’s work, but for now I think it’s more than enough to introduce such option as long as it’s not going to break original game gameplay rules.
Another little update. In order to manage enablement of one-way monoliths I’m started to move more logic into CGameInfoCallback. Though for now it’s not work exactly as I expected as my callback functions are called from within of CGTeleport, but even on client there is no player-specific “cb” available and I can’t check if player see object or no.
UPD: Sadly it’s even more complicated as it’s looks like “CPathfinder::calculatePaths” don’t have access to player-specific gamestate too… 
So I’m find few tricks to bypass that problem with player state. Not too pretty, but works.
Also I just implemented teleport channel exploration support for the VCAI. Now if AI hero going through bidirectional teleport channel and notice there is available exits which isn’t visible yet hero will instantly try to visit each of them and then teleport back to prior position.
So this also going to help the AI in situation of vision loss due to fog of war changes. E.g if channel have teleporters “A B C D” and for some reason vision on “B” was lost then even if hero just going from “A” to “C” he’ll check “B” too.
Improved teleport exploration of VCAI so:
[ul]]AI will remember blocked (no exit) teleport channels and will never attempt to visit entrances of such channel again./:m]
]AI with Explore goal will visit two-way teleport object (even if same entry was already visited) if any of exits of it’s channel are under FoW./:m][/ul]
Also find out about interesting H3 behaviours:
[ul]]If one exit blocked by friendly hero blocked then it’s will always teleport to different exits/:m]
]If all exits are blocked then it’s just do nothing. /:m]
]But for Subterranean Gate it’s will initiate exchange between heroes.
Also this exchange not using any movement points./:m][/ul]For some reason before I’m think like last behavior works for two-way Monolith’s too at least if they only have two exits…
Any thoughts on how this should be implemented in VCMI?
So I’m implemented almost all H3 behaviours except free hero exchange and added nice bonus feature to CPathfinder. As I posted before there is two separate options for unidirectional channels:
[ul]]allowTeleportOneWay: use channels with one known exit/:m]
]allowTeleportOneWayRandom: use channels with more than one known exit/:m][/ul]Now if there unidirectional channel with two exits and one exit is blocked by friendly hero pathfinder will treat it as channel with one exit and will build paths through it.
It’s would be good base to implement unidirectional teleporters exploration for the VCAI in future. E.g AI can leave heroes to stay on unidirectional channel exits while probe entrance using more heroes so it’s can check how many exits is there for sure.
PS: All new code is little bit hacky again, but I’ll go through few improvements passes once I’m sure that everything implemented.
Finally had time to test some behaviours @Ivan posted earlier.
Tested on map without taverns and towns.
In this in H3 it’s saying you “Unfortunately, it appears that the owner has left some guards and in VCMI behaviour is same. Do you wish to fight them?”. So it’s impossible to attack it accidentally.
In H3 it’s doesn’t show stacks in garrison, but does show “attack” cursor on it.
So it’s questionable if these are bugs in H3 or not and I have no idea what is valid way to implement it.
PS: Anyway if @Ivan still think this shouldn’t be implemented then I’m fine about that as it’s can be always merged separately later on and this will save me from digging into object creation process. In this case I’ll just add some teleport-specific code that allow/disallow transit.
I think you try to pack way too many features in one branch and it becomes very confusing. First finish one thing, test if it works and only then continue.
Let’s say fair I’m don’t really want to do that, but in same time I don’t want to merge code that going to break more things than improve. E.g transit feature appear not because “it’s cool and I want to do that”, but because there was real issue with accidental teleport visits that have to be solved. Same story about almost everything else.
Only thing I did “for fun only” was this Subterranean Gates for multiple layers, but I’m already removed that code for now. And I’m think I can leave very minimal transit code for teleporters only too.
Also I actually do test all code every time I rewrite anything.