CObjectHandler refactoring

International board Development
#1

I`ve started moving adv map mechanics from lib to server.

most of logic is onHeroVisit (and dependent functions), this code is being extracted to separate unit in server project. (Calls to not yet extracted objects may stay redirected to CObjectHandler, so incomplete refactoring should not break anything)

should this be in a branch or committed to trunk? (In branch it may be tested before merge, but any change in extracted code will cause conflicts)

#2

I’m not sure how are you actually implementing this but I don’t think that such move is desirable. Objects mechanics is strictly related to object state, that is game state and belongs to lib. I don’t see advantages of having things moved to server, I see several possible issues. Beginning with serializer issue “can’t store pointers to lib things in server things”.

Actaully, in a long-term plans I rather intended to move CGameHandler to lib and make server handle only networking and related issues.

#3

the only visible interface. everything else is hidden.

concrete Dispatcher manages a map ID->IServerObject for all IDs except random objects.

IServerObject descendants implements mechanic itself. (extracted Onxxx methods from ObjectHandler)

class IEventDispatcher
{
public:
	virtual ~IEventDispatcher();
	virtual void onHeroVisit(const CGObjectInstance *o, const CGHeroInstance *visitor) = 0;
	///other events 
	/**
	* Factory function
	* @param cb reference to GameHandler
	* @return Concrete instance of dispatecher
	*/
	static IEventDispatcher * createDispatcher(IGameCallback * cb);

};

base class for game objects

class IServerObject
{
public:
	IServerObject(CB &cb){this->cb = cb.cb; dispatcher = cb.dispatcher;}
	virtual ~IServerObject(){}
	
	virtual void onHeroVisit(const CGObjectInstance *o, const CGHeroInstance *visitor){}
	virtual void onHeroLeave(const CGObjectInstance *o, const CGHeroInstance *visitor){}
///other events 

protected:
	IGameCallback *cb;
	IEventDispatcher *dispatcher;
	
	void giveDummyBonus(const CGObjectInstance *o, int heroID, ui8 duration = Bonus::ONE_DAY) const;
};

and derived classes for all CGxxx classes.

There will be no issues with serializartion because all data remains in lib, only code is beeng moved.

Few more explanations.

I`m extracting code from ObjectHandler, that is only used by GameHandler. (If GameHandler will be moved to lib, my ServerObjectHandler can be moved to)

BTW I can put this code to lib right now (it has no dependencies from server)

#4

I also believe that adventure map objects require major refactoring.

Some things to consider:

  • Every distinct objects is aclass on its own. Common parameters are moved to abstract class up in hierarchy (for example VisitableOPW is abstract, while each of its kind gets differet sub class or parameters).
  • Unified interface, especially for AI. Objects on their own cna tell if they should be visited (done, but could be extended), what is their value, what is their danger or what kind of treasures they may give.
  • Factory functions move to object classes, instead of gargantuan CMapLoaderH3M::readObjects.

All these steps have a purpose of allowing new objects added to game, possibly with new classes inheriting or containing existing object classes.

Not sure what you are doing either, AVS, certainly have no idea what is the purpose of these changes. One important goal was to move game mechanics away from client and processing them on server only.

#5

My goal is at first to separate things that used by client|AI from things that only used by GameHandler.

I think that only factories, object specific data and readonly helpers (aka unified interface) should remain in ObjectHandler.

#6

@AVS
For me it still looks like introducing more complexity where it is not needed. I’m not sure what is the goal of detaching object mechanics from object state?
Could you please upload somewhere the patch, so I could get a full picture?

If I were to touch objects, I’d go in the opposite direction. Less classes but with more parameters. But I see no point in doing this. Hardcoded objects are for mimicking H3 behaviour. They are not meant to be extensible or generic. New objects will be handled by scripts.

Also, once I had idea for big objects refactoring. It was about creating a set of “atomic” (not decomposable in sense of mechanics) object behaviour classes that are as parametrized as possible. Then the actual objects would be composed from elemental pieces.
In the end however I ditched the idea as not effort-worthy.

We do not want AI (or generally anything on client) depend upon the actual presence of adventure map object classes instances on the client. It compromises the planned cheat-proof Multiplayer mode. I see current solution for “(not) visitied” as an evil workaround that should go.
I agree that there is a need for some kind of specification “what to expect” about objects. Not so much for the H3 objects that are known and can be hardcoded in AI but for the future mods. Such specification however should be available independently from particular object state (rather as a trait of object type, not particular object instance).

I wouldn’t move anything h3m-specific outside the h3m handling file. readObjects is gargantuan yet simple. Splitting this code among dozens of factories classes sounds rather as obfuscation. I know that “giant switch” is not object oriented, however sometimes it is good to stay simple.

#7

There is no patch ATM because existing code is not touched yet. In attachment current draft of extracted logic.

It is in fact logically detached already, because all changes are made through CallBack -> [server request] -> client packet -> GameState.

Current Scheme:
GameHandler -> ObjectHandler (methods) -> CallBack (=GameHandler ) -> Packet -> GameState -> ObjectHandler (data)

My propose:
GameHandler -> ServerObjectHandler (logical part of GameHandler in fact) -> [CallBack (=GameHandler )] -> Packet -> GameState -> ObjectHandler (data)
server.zip (16.1 KB)

#8

@AVS I don’t understand your proposal. Could you sketch an example? I’m curious what virtual void onHeroVisit(const CGObjectInstance *o, const CGHeroInstance *visitor){} is supposed to do in base class for objects. What is its first argument? What does dispatcher actually do? Why so many methods everywhere instead of hierarchy of Event classes? What would we gain once so much work is done (hint: “it will be more simple to do X”, “Y will be less error-prone”)?

I think that the next change in object handling should be aimed at cheat-proof multiplayer support, not extensibility or (doubtful) elegance.

EDIT: I see you posted something while I was writing it. My reply does not refer to it.

#9

I`m developing parallel hierarchy for event handling, so first parameter is object itself.

Ive finished extracting onHeroVisit, onHeroLeave, newTurn. Now objecthandler is splitted in two halves (about 4k lines each) and now I can reafctor event handling (as Warmonger wrote - separate class for each Obj::ID) without touching anything else - public interface remain stable. Even if this patch will not be accepted, Ill commit results of refactoring (backporting to objecthandler). There are many too huge methods and code duplication to refactor.

#10

wasVisited function is just an example and the final implementation should be improved to allow AI make plans based on object classes that are not visible yet on adventure map. The most important thing is to allow AI handle new objects automaticlaly, that is without need to rewrite AI itself.

Another thing I forgot to mention is RMG support for new objects - using same method. New objects should not only be able to tell RMG what is their value or group they belong to, but also very subjective info such as placement terrain or graphical style. It is impossible for any RMG algorithm to plan ahead placing objects in a way pleasant to eye. Object’s author should be free to define a number of variables that tell RMG what the object looks like and where it should be placed. Grouping together mountains and rocks of similiar color may be nice, but placing Forge-themed structures in a middle of lush jungle or magical garden will be wrong.

This shows the general principle that modding system follows - adding new items to game requires moving all possible related info to class of that item.

#11

I support Tow’s opinion that it shouldn’t be done. You showed nothing to convince us your solution will bring any benefit (I defined what I mean by benefit, see previous post), so I’ll support immediate revert (or even bother to do it myself).

#12

When I’ve created and placed towns for the RMG I had to deal with the object handler. My experience in this area is limited, but as I wrote that stuff I already thought how this could be improved. That’s what current town creation looks like:

CGTownInstance * town = new CGTownInstance();
town->ID = Obj::TOWN; // ID for a random town is also possible
town->subID = ETownType::CASTLE;
town->tempOwner = owner;
town->defInfo = VLC->dobjinfo->gobjs[town->ID][town->subID];
town->builtBuildings.insert(EBuilding::FORT);
town->builtBuildings.insert(-50); // standard buildings built

This piece of code creates a “standard town” with fort built and owned by the red player. With “standard town” I refer to how the OH3 map editor creates a town out-of-the-box. That generation could be improved like that:

CGTownInstance * town = new CGTownInstance(ETownType::CASTLE);
town->setOwner (0);

This is the same as above and if you want to create a random town first:

CGTownInstance * town = new CGTownInstance(); // defaults to ETownType::ANY, random town
town->setTownType(ETownType::CASTLE);

I don’t want to have factory methods here. Getters and setters could help a lot but would result in much refactoring. This is my proposal:

  • Could: Change ID and subID to protected and create a public getter where required. Change defInfo to protected and add public getter. (optional, can’t be changed anyway for a long time…)
  • Should: Create a descriptive method which automatically sets ID + subID + defInfo; e.g. setTownType(…) -> easier, less error-prone -> getTownType would return subId (better name)
  • Should: Default creation should be “default creation” as in OH3 map editor where possible

This doesn’t apply only to towns, but to other objects in the object handler as well. So my ideas are mostly minimal changes here and there, nothing special. @Tow: I’ll inform you if I’m going to change anything here:)

#13

@beegee

Finally good proposals in this thread :).

#14

Yes, detaching logic from state in OO design of itself may is not good, but mixing server-only (mechanics), client-only and common state is not good too. In theory

Detaching was side effect of complete hiding implementation behind general interface. Hiding made code to be easily refactored. Reafactored code is now at least more readable, and more compact.

#15

From what you showed us so far, I see you reimplemented virtual function dispatch using dictionary-based dispatcher and dozens of dynamic casts. This is just change for worse. Currently I don’t see any gain the redesign you’re proposing. Sorry. :frowning: To say for sure I’d have to see an actual patch but… I doubt.

There’s nothing wrong with server-only mechanics code in object. (However, there should be no client-only code in object. We don’t want client to depend on objects internal data.)

Besides, even though the code is only used by server, it’s functionality is not server specific. Translation between object event and its gameplay consequences is part of game logic.

It was already hidden. CGObjectInstance is general object interface. Implementation of object mechanics is hidden in derived classes. Server operates basically on CGObjectInstance* and don’t deal directly with implementation.
Introducing additional class between server and object doesn’t make implementation “more hidden” IMHO.

So far, apart from glue-code (dispatcher, new clases, etc) the only difference I saw is that in methods you use dynamic_cast(obj)->myField instead of myField (that is avaialble in “this” ptr when mechanics is with object state).
It didn’t seem more readable to me and certainly was not more compact. For object I need now two clases, instead of one. (plus additional type registering place) To know about object I need to read two classes in two files.
Unless you done some significant changes since last draft?

#16

“more readable, and more compact” is refactored code and not just extracted. Significant change is recaftoring itself. (At most Get rid of code duplication, magic numbers, use polymorphism instead of large switch blocks)

#17

Now that are righteous causes. :slight_smile:
Code duplication is root of all evil.
Excessive usage of magic numbers in VCMI was my big mistake.
Polymorphism instead of switches — ok, just don’t overdo. [As a side note, it is much easier in some object instance to change ID than type. Fortunately, this is not typical use case. Only prisons need that AFAIR.]

Can we get refactoring part and not the split?

#18

Yes, after split I made Hero and Prison separate classes :slight_smile:

I`m backporting some refactorings (only code dedup and magic numbers) to trunk right now.