namidaka / crpg

Multiplayer mod for Mount & Blade II: Bannerlord
https://c-rpg.eu
GNU General Public License v3.0
14 stars 14 forks source link

mod: keep value when generating stats for fixed price items #355

Closed Muparadzi closed 3 months ago

Muparadzi commented 3 months ago

@Meowkov reported issues with the exporter removing the 'value' field in weapons.xml when setting smokebomb price manually. This change removes that functionality, so crafters can set price overrides without it impacting the xml.

Meowkov commented 3 months ago

If I understand this right this means there is no check anymore on prices, so we need to be double checking the prices of all items going forwards during PR review to ensure there haven't been any mistakes made, is that right?

This is kinda correct if there is a value= variable on the item/armor. Usually items will not have the value= variable at all so if it shows up in any of the items xmls, we need to check if it is intended. It would mostly be me copy pasting xml stuff over from another mod when importing items and forgetting to remove it, crafters should never add it on their own.

All items that do not have the value= variable will still be auto calculated as before on export.

Yeldur commented 3 months ago

If I understand this right this means there is no check anymore on prices, so we need to be double checking the prices of all items going forwards during PR review to ensure there haven't been any mistakes made, is that right?

This is kinda correct if there is a value= variable on the item/armor. Usually items will not have the value= variable at all so if it shows up in any of the items xmls, we need to check if it is intended. It would mostly be me copy pasting xml stuff over from another mod when importing items and forgetting to remove it, crafters should never add it on their own.

All items that do not have the value= variable will still be auto calculated as before on export.

So in other words it's not something that is likely to be accidentally done by a crafter by mistake and something we'd need to perform a check on when reviewing PRs?

That's basically my only concern here, if it's just a case that when those changes are put INTO PR's we'd need to check them, that's totally fine, otherwise I'd want to iron out checks on things to be certain.

Meowkov commented 3 months ago

If I understand this right this means there is no check anymore on prices, so we need to be double checking the prices of all items going forwards during PR review to ensure there haven't been any mistakes made, is that right?

This is kinda correct if there is a value= variable on the item/armor. Usually items will not have the value= variable at all so if it shows up in any of the items xmls, we need to check if it is intended. It would mostly be me copy pasting xml stuff over from another mod when importing items and forgetting to remove it, crafters should never add it on their own. All items that do not have the value= variable will still be auto calculated as before on export.

So in other words it's not something that is likely to be accidentally done by a crafter by mistake and something we'd need to perform a check on when reviewing PRs?

That's basically my only concern here, if it's just a case that when those changes are put INTO PR's we'd need to check them, that's totally fine, otherwise I'd want to iron out checks on things to be certain.

Exactly, someone would have to add value="12345" to an item in the xmls (one piece weapons, bows, xbows, armors) so this can not happen on accident and will be pretty obvious in a PR diff view 👍