feature_WorkerExpGainedOnCraft

[Centorios]

-Added variables to change "per crafting material" exp bonus while manufacturing items
-Added quantity multiplier to manufacturing exp gain

[Centorios]

1. Widened t_UserOBJ.Amount:
   Inventory and banking logic widely read/write Object(Slot).amount. Because VB6 is case-insensitive, “amount” references continue to compile. However, the struct’s memory layout changes (Integer→Long), which can affect:
   Any raw serialization of t_UserOBJ via Put/Get/LSet/RtlMoveMemory. We did not find direct raw Put/Get of t_UserOBJ, and protocol writers seem to write fields explicitly, so the risk appears low. Still, review any storage/network code touching t_UserOBJ in bulk.

2. Common usage examples that still reference .amount:
   NPC and user commerce, banking, quest rewards, treasure/regalo loaders, starter kits, fishing delivery, trading with users.
   These will all benefit from the widened Long capacity but increase memory footprint of inventory arrays.

3. Long quantity parameters for TieneObjetos/QuitarObjetos/CarpinteroQuitarMateriales:
   All call sites now accept larger counts without overflow. VB6 will widen Integer arguments implicitly, so call sites need not change, but any caller relying on Integer overflow would behave differently.
   High counts (>32,767) could previously overflow; now they won’t, potentially exposing other limits such as GetMaxInvOBJ (often 10,000) or server-side anti-cheat checks.

4. GiveExpWhileWorking changes:
   Signature now supports Quantity and aggregates per-material exp. tmpExp widened from Byte to Long prevents overflow when summing multiple per-material components.
   Call sites in crafting updated to pass MiObj.ObjIndex and MiObj.Amount, notably in HerreroConstruirItem/CarpinteroConstruirItem/AlquimistaConstruirItem, which changes the exp source from a generic tool to the crafted item’s composition and quantity.
   Other jobs (mining, woodcutting, fishing) continue to call the function without Quantity, relying on MiningExp/FellingExp/FishingExp; optional parameter preserves compatibility.

Potential effects and risks to review

1. Serialization/Marshalling:
   If any code writes/reads t_UserOBJ as a binary block (e.g., Put #, LSet, CopyMemory on the entire struct), the size/layout change will break persistence/protocol compatibility. Our searches did not find direct raw Put/Get of t_UserOBJ, but please review:
   Save/load routines for users and inventories.
   Network packet writers for inventory/bank slot updates (e.g., WriteChangeInventorySlot, WriteChangeBankSlot) to ensure field-wise serialization is used, not raw struct copies.

2. Memory footprint:
   t_UserOBJ grows by 2 bytes for the amount field (Integer→Long). Arrays like invent.Object(1..MAX_INVENTORY_SLOTS) and BancoInvent.Object will consume more memory. Unlikely critical, but note for very large MaxUsers.

3. Logic limits:
   Many routines cap quantities via GetMaxInvOBJ or hardcoded limits (like 10,000 in player-to-player trade). Widening underlying Long may allow internal intermediate calculations beyond old range; verify:
   Anti-cheat checks (e.g., commerce ban when buying too many items).
   UI/packet representations: ensure protocol uses 32-bit for quantities where needed, and client can handle numbers up to the new limits.

4. Configuration consistency:
   The branch expects per-material job exp keys; ensure Configuracion.ini is deployed with IronForgingExp, SilverForgingExp, GoldForgingExp, BlodiumForgingExp, WoodCarpentryExp, ElvenWoodCarpentryExp present, or ServerConfig defaults cover absence.
   tmpExp remains aggregated as Long but underlying SvrConfig values are read via CByte; this is fine, but large aggregates should remain below Long bounds. Confirm Stats.Exp type is Long to avoid overflow of accumulated exp.
   Recommendations

5. Confirm persistence and protocol writers:
   Inspect implementations of WriteChangeInventorySlot, WriteChangeBankSlot and any save/load routines for inventory. Ensure they serialize fields individually. If any bulk CopyMemory/LSet is found targeting t_UserOBJ, refactor to field-wise serialization.

6. Add guardrails for quantity handling:
   Where appropriate, clamp Long quantities at logical limits using GetMaxInvOBJ or similar constants. Review commerce, bank, trading, quest reward delivery, and starter kit logic to ensure they enforce caps uniformly.
   Validate exp accumulation:
   With GiveExpWhileWorking now aggregating per material, craft multi-quantity items to test that exp awarded matches expectations and doesn’t overflow or underflow. Consider unit/integration tests around blacksmith/carpenter/alchemist recipes.

Useful links to continue the review

The commit comparison used for this analysis:
master...feature_WorkerExpGainedOnCraft
Branch commits (results may be incomplete due to API limits):
View more: https://github.com/ao-org/argentum-online-server/commits/feature_WorkerExpGainedOnCraft
Code search references used (results may be incomplete; use these to broaden review in the UI):

amount field usages: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+amount&type=code
TieneObjetos calls: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+TieneObjetos%28&type=code
QuitarObjetos calls: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+QuitarObjetos%28&type=code
GiveExpWhileWorking calls: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+GiveExpWhileWorking%28&type=code
WriteChangeInventorySlot: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+WriteChangeInventorySlot&type=code
WriteChangeBankSlot: https://github.com/search?q=repo%3Aao-org%2Fargentum-online-server+WriteChangeBankSlot&type=code

[Centorios]

@morgolock revisar con ganas.

[morgolock]

Muchos problemas a solucionar, requiere rework

[morgolock]

Refactor this to pass MiObj by ref and Call GiveExpWhileWorking(UserIndex, MiObj, e_JobsTypes.Blacksmith)

[morgolock]

Refactor this to GiveExpWhileWorking(ByVal UserIndex As Integer, obj as t_Obj, ByVal JobType As Byte)

3. GiveExpWhileWorking signature and types: Quantity is a Long, and default type mismatch

You changed:

Public Function GiveExpWhileWorking(... Optional ByVal Quantity As Integer = 1)

But you’re passing MiObj.Amount which is now Long.

In VB6 this will coerce, but it’s still wrong-footed and can overflow if Amount > 32767.

Action: make it:

Optional ByVal Quantity As Long = 1

and keep tmpExp As Long (good change).

[morgolock]

1. t_UserOBJ.amount As Integer → Amount As Long is a breaking structural change

Changing a UDT field type (and even just its name, though VB is case-insensitive) changes the binary layout of t_UserOBJ.

That can break anything that:

Get/Put’s inventory structs to disk (binary saves, charfiles, etc.)

Sends inventory structs over the network (packet layouts)

Copies memory blobs around (e.g., CopyMemory, RtlMoveMemory, custom serialization)

Even if you never serialize t_UserOBJ, a UDT size change can still break code that assumes offsets.

Action: search for any Get #, Put #, Len(t_UserOBJ), CopyMemory, or packet building that writes t_UserOBJ raw. If any exist, you need a migration/compat layer or keep stored format as Integer and clamp.

Also: Inventory stack sizes being Long is reasonable, but you should enforce caps anyway (anti-dupe / overflow / economy).

[morgolock]

2. QuitarObjetos and material removal types: mixed Long/Integer

You updated:

QuitarObjetos(... cant As Long ...)

But left:

CarpinteroQuitarMateriales(... CantidadElfica As Integer, CantidadPino As Integer)

If someone crafts in bulk and CantidadElfica or CantidadPino can exceed 32767, that’s a silent overflow risk.

Action: make all these quantities Long for consistency:

CantidadElfica As Long

CantidadPino As Long

(and any other “Cantidad*” used in bulk crafting paths)

[morgolock]

6. Duplicate Case e_JobsTypes.Woodcutter

You currently have Woodcutter twice:

Case e_JobsTypes.Woodcutter
tmpExp = ...
...
Case e_JobsTypes.Woodcutter
tmpExp = ...

Not harmful (first match wins), but it’s dead/duplicate code and can hide future edits.

Action: remove the duplicate case.

[morgolock]

4. Big logic bug in Alchemist EXP: copy/paste uses the wrong ingredient every time

You added:

If ObjData(ItemIndex).FlorOceano > 0 Then
tmpExp = tmpExp + ObjData(ItemIndex).FlorRoja * SvrConfig.GetValue("MixingExp")
End If

…and similarly for ColaDeZorro, Tuna, HongoDeLuz, SemillasPros, Cala: they all add FlorRoja.

That’s almost certainly a bug.

Fix: each branch should multiply its own field:

FlorOceano branch uses ObjData(ItemIndex).FlorOceano

ColaDeZorro uses ObjData(ItemIndex).ColaDeZorro

etc.

Also: you don’t apply * Quantity in the Alchemist case (you do for forging/carpentry). If bulk crafting is intended to scale EXP, you likely want * Quantity for Alchemist too.