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

optimistic #2220

Merged
merged 15 commits into from
Dec 5, 2024
4 changes: 4 additions & 0 deletions client/src/dojo/createClientComponents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,9 @@ export function createClientComponents({ contractComponents }: SetupNetworkResul
Position: overridableComponent(contractComponents.Position),
Stamina: overridableComponent(contractComponents.Stamina),
Tile: overridableComponent(contractComponents.Tile),
Population: overridableComponent(contractComponents.Population),
Resource: overridableComponent(contractComponents.Resource),
Weight: overridableComponent(contractComponents.Weight),
Army: overridableComponent(contractComponents.Army),
};
}
103 changes: 91 additions & 12 deletions client/src/dojo/modelManager/ArmyManager.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,97 @@
import { DojoResult } from "@/hooks/context/DojoContext";
import { ID } from "@bibliothecadao/eternum";
import { ID, ResourcesIds } from "@bibliothecadao/eternum";
import { getComponentValue } from "@dojoengine/recs";
import { getEntityIdFromKeys } from "@dojoengine/utils";
import { uuid } from "@latticexyz/utils";
import { ResourceManager } from "./ResourceManager";

interface Troops {
[ResourcesIds.Paladin]: number;
[ResourcesIds.Crossbowman]: number;
[ResourcesIds.Knight]: number;
}

export class ArmyManager {
constructor(private dojo: DojoResult) {}

async deleteArmy(armyId: ID): Promise<void> {
await this.dojo.setup.systemCalls
.delete_army({
signer: this.dojo.account.account,
army_id: armyId,
})
.then(() => {
this.dojo.network.world.deleteEntity(getEntityIdFromKeys([BigInt(armyId)]));
});
private readonly realmEntityId: number;

constructor(
private readonly dojo: DojoResult,
private readonly armyEntityId: ID,
) {
this.realmEntityId = this.getRealmEntityId();
}
Comment on lines +17 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle Potential Undefined Realm Entity ID

In the constructor, this.realmEntityId is assigned using this.getRealmEntityId(), which defaults to 0 if the entity_owner_id is undefined. Ensure that 0 is a valid value in this context, or implement error handling for cases where entity_owner_id is not found.

Consider adding a check and throwing an error or warning if entity_owner_id is undefined.


private getRealmEntityId(): number {
return (
getComponentValue(this.dojo.setup.components.EntityOwner, getEntityIdFromKeys([BigInt(this.armyEntityId)]))
?.entity_owner_id || 0
);
}

private _updateResourceBalances(overrideId: string, troops: Troops): void {
Object.entries(troops).forEach(([troopType, amount]) => {
const resourceManager = new ResourceManager(
this.dojo.setup,
this.realmEntityId,
Number(troopType) as ResourcesIds,
);
resourceManager.optimisticResourceUpdate(overrideId, -BigInt(amount));
});
}

private _updateArmyTroops(overrideId: string, army: any, troops: Troops): void {
this.dojo.setup.components.Army.addOverride(overrideId, {
entity: getEntityIdFromKeys([BigInt(this.armyEntityId)]),
value: {
...army,
troops: {
knight_count: BigInt(army.troops.knight_count) + BigInt(troops[ResourcesIds.Knight]),
crossbowman_count: BigInt(army.troops.crossbowman_count) + BigInt(troops[ResourcesIds.Crossbowman]),
paladin_count: BigInt(army.troops.paladin_count) + BigInt(troops[ResourcesIds.Paladin]),
Comment on lines +48 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check for Potential Integer Overflows

When updating troop counts in _updateArmyTroops, ensure that adding BigInt values does not cause integer overflows.

Consider adding checks to prevent overflows:

+// Check for potential overflows
+const newKnightCount = BigInt(army.troops.knight_count) + BigInt(troops[ResourcesIds.Knight]);
+if (newKnightCount > MAX_SAFE_INTEGER) {
+  throw new Error('Knight count exceeds maximum safe integer.');
+}

Committable suggestion skipped: line range outside the PR's diff.

},
},
});
}

private _optimisticAddTroops(overrideId: string, troops: Troops): void {
const entity = getEntityIdFromKeys([BigInt(this.armyEntityId)]);
const army = getComponentValue(this.dojo.setup.components.Army, entity);

if (!army) return;

this._updateResourceBalances(overrideId, troops);
this._updateArmyTroops(overrideId, army, troops);
}

public addTroops(troops: Troops): void {
this.dojo.setup.systemCalls.army_buy_troops({
signer: this.dojo.account.account,
payer_id: this.realmEntityId,
army_id: this.armyEntityId,
troops: {
knight_count: BigInt(troops[ResourcesIds.Knight]),
crossbowman_count: BigInt(troops[ResourcesIds.Crossbowman]),
paladin_count: BigInt(troops[ResourcesIds.Paladin]),
},
});

this._optimisticAddTroops(uuid(), troops);
}
Comment on lines +66 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle Asynchronous System Call in addTroops

The addTroops method calls army_buy_troops but does not handle the promise or any potential errors. This might lead to unhandled promise rejections.

Update the method to handle the asynchronous operation:

-public addTroops(troops: Troops): void {
+public async addTroops(troops: Troops): Promise<void> {
    try {
      await this.dojo.setup.systemCalls.army_buy_troops({
        // parameters...
      });
      this._optimisticAddTroops(uuid(), troops);
    } catch (error) {
      // Handle error appropriately
      console.error('Error adding troops:', error);
    }
  }

Committable suggestion skipped: line range outside the PR's diff.


public createArmy(structureEntityId: bigint, isDefensive: boolean): void {
this.dojo.setup.systemCalls.create_army({
signer: this.dojo.account.account,
is_defensive_army: isDefensive,
army_owner_id: structureEntityId,
});
}

public async deleteArmy(armyId: ID): Promise<void> {
await this.dojo.setup.systemCalls.delete_army({
signer: this.dojo.account.account,
army_id: armyId,
});

this.dojo.network.world.deleteEntity(getEntityIdFromKeys([BigInt(armyId)]));
}
}
59 changes: 54 additions & 5 deletions client/src/dojo/modelManager/ArmyMovementManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
ContractAddress,
ID,
ResourcesIds,
TravelTypes,
getDirectionBetweenAdjacentHexes,
getNeighborHexes,
} from "@bibliothecadao/eternum";
Expand Down Expand Up @@ -220,6 +221,18 @@ export class ArmyMovementManager {
});
};

private readonly _optimisticCapacityUpdate = (overrideId: string, capacity: number) => {
const currentWeight = getComponentValue(this.setup.components.Weight, this.entity);

this.setup.components.Weight.addOverride(overrideId, {
entity: this.entity,
value: {
entity_id: this.entityId,
value: (currentWeight?.value || 0n) + BigInt(capacity),
},
});
};

private readonly _optimisticTileUpdate = (overrideId: string, col: number, row: number) => {
const entity = getEntityIdFromKeys([BigInt(col), BigInt(row)]);

Expand Down Expand Up @@ -252,6 +265,12 @@ export class ArmyMovementManager {
this._optimisticStaminaUpdate(overrideId, configManager.getExploreStaminaCost(), currentArmiesTick);
this._optimisticTileUpdate(overrideId, col, row);
this._optimisticPositionUpdate(overrideId, col, row);
this._optimisticCapacityUpdate(
overrideId,
// all resources you can find have the same weight as wood
configManager.getExploreReward() * configManager.getResourceWeight(ResourcesIds.Wood),
);
this._optimisticFoodCosts(overrideId, TravelTypes.Explore);

return overrideId;
};
Expand All @@ -277,18 +296,21 @@ export class ArmyMovementManager {
signer: useAccountStore.getState().account!,
})
.catch((e) => {
this.setup.components.Position.removeOverride(overrideId);
this.setup.components.Tile.removeOverride(overrideId);
// remove all visual overrides only when the action fails
this._removeVisualOverride(overrideId);
this._removeNonVisualOverrides(overrideId);
})
.then(() => {
this.setup.components.Stamina.removeOverride(overrideId);
// remove all non visual overrides
this._removeNonVisualOverrides(overrideId);
});
};

private readonly _optimisticTravelHex = (col: number, row: number, pathLength: number, currentArmiesTick: number) => {
const overrideId = uuid();

this._optimisticStaminaUpdate(overrideId, configManager.getTravelStaminaCost() * pathLength, currentArmiesTick);
this._optimisticFoodCosts(overrideId, TravelTypes.Travel);

this.setup.components.Position.addOverride(overrideId, {
entity: this.entity,
Expand All @@ -301,6 +323,32 @@ export class ArmyMovementManager {
return overrideId;
};

// only remove visual overrides (linked to models on world map) when the action fails
private readonly _removeVisualOverride = (overrideId: string) => {
this.setup.components.Tile.removeOverride(overrideId);
this.setup.components.Position.removeOverride(overrideId);
};

// you can remove all non visual overrides when the action fails or succeeds
private readonly _removeNonVisualOverrides = (overrideId: string) => {
this.setup.components.Stamina.removeOverride(overrideId);
this.setup.components.Resource.removeOverride(overrideId);
this.setup.components.Weight.removeOverride(overrideId);
};

private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => {
const entityArmy = getComponentValue(this.setup.components.Army, this.entity);
let costs = { wheatPayAmount: 0, fishPayAmount: 0 };
if (travelType === TravelTypes.Explore) {
costs = computeExploreFoodCosts(entityArmy?.troops);
} else {
costs = computeTravelFoodCosts(entityArmy?.troops);
}

this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount)));
this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount)));
};
Comment on lines +339 to +350
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential undefined entityArmy to prevent runtime errors

In the _optimisticFoodCosts method, entityArmy may be undefined. Accessing entityArmy.troops without checking can lead to runtime errors. Please add a check to ensure entityArmy is defined before proceeding.

Apply this diff to handle the potential undefined entityArmy:

private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => {
  const entityArmy = getComponentValue(this.setup.components.Army, this.entity);
+ if (!entityArmy) {
+   // Handle undefined entityArmy, possibly by returning early or setting default costs
+   return;
+ }
  let costs = { wheatPayAmount: 0, fishPayAmount: 0 };
  if (travelType === TravelTypes.Explore) {
    costs = computeExploreFoodCosts(entityArmy.troops);
  } else {
    costs = computeTravelFoodCosts(entityArmy.troops);
  }
  // Rest of the code...
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => {
const entityArmy = getComponentValue(this.setup.components.Army, this.entity);
let costs = { wheatPayAmount: 0, fishPayAmount: 0 };
if (travelType === TravelTypes.Explore) {
costs = computeExploreFoodCosts(entityArmy?.troops);
} else {
costs = computeTravelFoodCosts(entityArmy?.troops);
}
this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount)));
this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount)));
};
private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => {
const entityArmy = getComponentValue(this.setup.components.Army, this.entity);
if (!entityArmy) {
// If there's no army, no resources need to be consumed
return;
}
let costs = { wheatPayAmount: 0, fishPayAmount: 0 };
if (travelType === TravelTypes.Explore) {
costs = computeExploreFoodCosts(entityArmy.troops);
} else {
costs = computeTravelFoodCosts(entityArmy.troops);
}
this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount)));
this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount)));
};


private readonly _travelToHex = async (path: HexPosition[], currentArmiesTick: number) => {
const overrideId = this._optimisticTravelHex(
path[path.length - 1].col,
Expand All @@ -326,10 +374,11 @@ export class ArmyMovementManager {
directions,
})
.catch(() => {
this.setup.components.Position.removeOverride(overrideId);
this._removeVisualOverride(overrideId);
this._removeNonVisualOverrides(overrideId);
})
.then(() => {
this.setup.components.Stamina.removeOverride(overrideId);
this._removeNonVisualOverrides(overrideId);
});
};

Expand Down
12 changes: 12 additions & 0 deletions client/src/dojo/modelManager/ResourceManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,18 @@ export class ResourceManager {
return finishTick > currentTick ? finishTick - currentTick : 0;
}

public optimisticResourceUpdate = (overrideId: string, change: bigint) => {
const entity = getEntityIdFromKeys([BigInt(this.entityId), BigInt(this.resourceId)]);
const currentBalance = getComponentValue(this.setup.components.Resource, entity)?.balance || 0n;
this.setup.components.Resource.addOverride(overrideId, {
entity,
value: {
resource_type: this.resourceId,
balance: currentBalance + change,
},
});
};

public timeUntilValueReached(currentTick: number, value: number): number {
const production = this._getProduction(this.resourceId);
const resource = this._getResource(this.resourceId);
Expand Down
81 changes: 68 additions & 13 deletions client/src/dojo/modelManager/TileManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import {
ResourcesIds,
StructureType,
} from "@bibliothecadao/eternum";
import { getComponentValue, Has, HasValue, NotValue, runQuery } from "@dojoengine/recs";
import { Entity, getComponentValue, Has, HasValue, NotValue, runQuery } from "@dojoengine/recs";
import { uuid } from "@latticexyz/utils";
import { AccountInterface, CairoOption, CairoOptionVariant } from "starknet";
import { SetupResult } from "../setup";
import { configManager, SetupResult } from "../setup";

export class TileManager {
private col: number;
Expand Down Expand Up @@ -198,10 +198,44 @@ export class TileManager {
paused: false,
},
});

const populationOverrideId = uuid();

const realmEntityId = getEntityIdFromKeys([BigInt(entityId)]);

this.setup.components.Population.addOverride(populationOverrideId, {
entity: realmEntityId,
value: {
population:
(getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) +
configManager.getBuildingPopConfig(buildingType).population,
},
});

const resourceChange = configManager.buildingCosts[buildingType];

resourceChange.forEach((resource) => {
this._overrideResource(realmEntityId, resource.resource, -BigInt(resource.amount));
});

return overrideId;
};

private _overrideResource = (entity: Entity, resourceType: number, change: bigint) => {
const currentBalance = getComponentValue(this.setup.components.Resource, entity)?.balance || 0n;
const resourceOverrideId = uuid();
this.setup.components.Resource.addOverride(resourceOverrideId, {
entity,
value: {
resource_type: resourceType,
balance: currentBalance + change,
},
});
};

private _optimisticDestroy = (entityId: ID, col: number, row: number) => {
const currentBuilding = getComponentValue(this.setup.components.Building, getEntityIdFromKeys([BigInt(entityId)]));

const overrideId = uuid();
const realmPosition = getComponentValue(this.setup.components.Position, getEntityIdFromKeys([BigInt(entityId)]));
const { x: outercol, y: outerrow } = realmPosition || { x: 0, y: 0 };
Expand All @@ -220,6 +254,20 @@ export class TileManager {
outer_entity_id: 0,
},
});

const populationOverrideId = uuid();

const realmEntityId = getEntityIdFromKeys([BigInt(entityId)]);

this.setup.components.Population.addOverride(populationOverrideId, {
entity: realmEntityId,
value: {
population:
(getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) -
configManager.getBuildingPopConfig(currentBuilding?.category as unknown as BuildingType).population,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle undefined 'currentBuilding' to prevent runtime errors

In the _optimisticDestroy method, currentBuilding may be undefined if the building does not exist. Accessing currentBuilding.category when currentBuilding is undefined will lead to a runtime error. Ensure that currentBuilding is defined before using its properties.

You can modify the code to check if currentBuilding is defined:

if (currentBuilding?.category !== undefined) {
  const buildingPopulation = configManager.getBuildingPopConfig(currentBuilding.category as BuildingType).population;
  this.setup.components.Population.addOverride(populationOverrideId, {
    entity: realmEntityId,
    value: {
      population:
        (getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) -
        buildingPopulation,
    },
  });
} else {
  // Handle the case where currentBuilding is undefined
  // Possibly log an error or take appropriate action
}

},
});

return overrideId;
};

Expand Down Expand Up @@ -277,21 +325,28 @@ export class TileManager {
const directions = getDirectionsArray(startingPosition, endPosition);

// add optimistic rendering
const _ = this._optimisticBuilding(entityId, col, row, buildingType, resourceType);
const overrideId = this._optimisticBuilding(entityId, col, row, buildingType, resourceType);
const { isSoundOn, effectsLevel } = useUIStore.getState();

playBuildingSound(buildingType, isSoundOn, effectsLevel);

await this.setup.systemCalls.create_building({
signer: useAccountStore.getState().account!,
entity_id: entityId,
directions: directions,
building_category: buildingType,
produce_resource_type:
buildingType == BuildingType.Resource && resourceType
? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
: new CairoOption<Number>(CairoOptionVariant.None, 0),
});
try {
await this.setup.systemCalls.create_building({
signer: useAccountStore.getState().account!,
entity_id: entityId,
directions: directions,
building_category: buildingType,
produce_resource_type:
buildingType == BuildingType.Resource && resourceType
? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
: new CairoOption<Number>(CairoOptionVariant.None, 0),
Comment on lines +367 to +368
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use 'number' instead of 'Number' for primitive types

The static analysis tool indicates that 'Number' should not be used as a type. For consistency and to prevent potential issues, please use the lowercase 'number' primitive type.

Apply this diff to correct the issue:

-              ? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
-              : new CairoOption<Number>(CairoOptionVariant.None, 0),
+              ? new CairoOption<number>(CairoOptionVariant.Some, resourceType)
+              : new CairoOption<number>(CairoOptionVariant.None, 0),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
: new CairoOption<Number>(CairoOptionVariant.None, 0),
? new CairoOption<number>(CairoOptionVariant.Some, resourceType)
: new CairoOption<number>(CairoOptionVariant.None, 0),
🧰 Tools
🪛 Biome (1.9.4)

[error] 341-341: Don't use 'Number' as a type.

Use lowercase primitives for consistency.
Safe fix: Use 'number' instead

(lint/complexity/noBannedTypes)


[error] 342-342: Don't use 'Number' as a type.

Use lowercase primitives for consistency.
Safe fix: Use 'number' instead

(lint/complexity/noBannedTypes)

});
} catch (error) {
this._optimisticBuilding(entityId, col, row, BuildingType.None, resourceType);

console.error(error);
throw error;
}
};

destroyBuilding = async (col: number, row: number) => {
Expand Down
Loading
Loading