vcmi icon indicating copy to clipboard operation
vcmi copied to clipboard

Refactor new turn processing on server

Open IvanSavenko opened this issue 1 year ago • 0 comments

Should have no functionality changes. Reorganized code for better readability and simplicity.

  • Moved most of code from CGameHandler::onNewTurn() into a separate class, NewTurnProcessor
  • Split former onNewTurn() method into a set of smaller methods with a single function for each.
  • Added IOwnableObject interface that is implemented by all objects that can be owned by a playet
  • CPlayerState now stores all owned objects, without separating them by type (towns/heroes/dwellings). Other owned objects like mines or garrisons are now also stored in CPlayerState, removing need to interate over all map objects to (for example) find all mines owned by a player
  • All objects that give fixed income now have method dailyIncome() that is used by NewTurnProcessor to give resources on new day. Removed CGMine::newTurn() method.

IvanSavenko avatar Aug 26 '24 19:08 IvanSavenko

I like that kind of backward compatibility. It is now one snap item per laser for every version. So no need to look at two snap items to get the full laser details. I am not sure if it is what @heinrich5991 suggested but the way I understood his idea was to only put the new fields in an additional item while still sending the old item for the position of the laser.

The code is a bit more complicated now but it can be cleaned up in 0.8 and until then it's like a soft deprecation of the old laser item.

I am just not sure about the name "Ex" what happens if we want to extend the laser again in 0.7? Will it be "Ex2"?

ChillerDragon avatar Aug 10 '24 17:08 ChillerDragon

I am just not sure about the name "Ex" what happens if we want to extend the laser again in 0.7? Will it be "Ex2"?

Well I don't see in which way it could be extended tbh.

CherryEx avatar Aug 10 '24 17:08 CherryEx

I just found out that the backwards compatibility isn't working; and there is a blackout so I can't work on it atm. First problem: I'm sending CNetObj_LaserEx with NETOBJTYPE_LASER instead of LASEREX Second problem: Because I used NETOBJTYPE_LASER for the new laser, now the old laser would not be recognized anymore. (To fix this I need to make RenderLaser accept both CNetObj_Laser and LaserEx but I'm not sure which method would be the best)

CherryEx avatar Aug 10 '24 19:08 CherryEx

Ok it's ready now. NOTICE: This is meant to be merged in 0.7.6 for the compatibility to work properly.

CherryEx avatar Aug 10 '24 22:08 CherryEx

I am not sure if it is what @heinrich5991 suggested

This is precisely what I wanted to suggest.

heinrich5991 avatar Aug 12 '24 10:08 heinrich5991

Why don't we do like commit #73f7182e50ddd9a9b6fac2d7ee93089e3d056673?

Bamcane avatar Aug 17 '24 23:08 Bamcane