I think that pathfinding may suggest you to do that, but not enforce that choice. From my point of view it’s would be perfect if you can click on any point on map and pathfinder will suggest the path through pickable objects, guards (for example if they weaker than you), water, etc. As @AVS suggested it’s may be something like Ctrl+click or Shift+click.

It’s doesn’t mean that everyone will want to use it every time, but it’s would be really useful because somethings it’s quite hard to find entry of some area of find what monster is blicking entry.

In case of AI it’s may be useful in case if goal is to get grail or certain artifact.

Got it. Though any example would be helpful.
Let’s say fair object loading code is quite complicated.

All code in doMoveHero is really hacky at moment. I’m wanna rewrite it as well as CPathfinder, but I think this should be done only once all functionality implemented. New movement system (one we discussed on github), water walking and flying going to add even more complexity here.

So I think it’s better to find out all use cases and only then rewrite it.

Yeah. On the other hand - this is as simple as I could get it without completely breaking H3 behavior.
wiki.vcmi.eu/index.php?title=Object_Format

Idea is that

1. There is object group always using same handler class
2. Object group contains list of object types and “base” entry to save typing for common entries.
3. Each object type contains list of templates and “base” entry. ( and this “base” may be inherited from object group “base” entry)
4. If there are no defined templates - VCMI will try to import this data from H3

For your case you need something like this:

"someMonolith" : {
	"index" : 12345,		// unique ID
	"handler": "teleport",	// handler ID
	"base" : {				// entry that will be added to everything in "types", optional
		"myCoolEntry" : "something", 
		
		"base" : {			// entry that will be added to everything in "templates", optional
			"visitableFrom" :  "---", "+++", "+++" ],
		}
	},
	"types" : {
		"sometype" : {
			"index" : 0		// object subtype
							// no "myCoolEntry" here - it will be inherited from "base"
		},
		"anothertype" : {
			"index" : 1,	
			"myCoolEntry" : "not something" // or if you want - you can override this on per-object basis (or set it in each object and forget about "base")
			}
		}
	}
},

In this example object handler will be created twice, once for “sometype” and another for “anothertype”. But in both case it will receive “myCoolEntry” field - before passing data to handler VCMI will merge object type description with base entry.

IIRC you’ll need to create custom handler for your class - generic handler can only be used for properly-free classes.

Thanks. Will do next attempt to implement it. Though only thing I worry about is fact that I don’t want to replace H3 templates.

You don’t have to. This is one of ideas behind this structure.

If your object does not provides “templates” entry VCMI will try to import them from H3. Structure from example above will work fine - types don’t have templates defined so it won’t break h3 objects

UPD:
Why do we have that loop in doMoveHero?

Perhaps a better approach would be something like this:

1. player requests hero movement -> client sends moveHero request & blocks UI
2. client receives reply from server -> moves hero according to reply
3. if path is not empty and interruption was not requested -> send moveHero again
4. Goto 2

So instead of such loop there would be ping-pong between server and client till player will request stop or hero reaches his destination/out of MP.

Oh - and as for “extended pathfinding” - IF you’re going to implement it then it should use any possible means of travel including battles ( passing through enemy heroes & neutrals) in addition to things like spells/one-way monoliths. Main purpose would be not to choose route but to show player possible way to reach destination.

One way monoliths is already working and there really nothing wrong with using them as long as we know there only one exit. Everything else is just idea that may be implemented, but only after more important things like flying/water walking are ready.

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:

1. Normal pathfinding - it will only go through some locations like owned/empty garrisons & two-way teleports. That’s it - 100% safe options.
2. 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.