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

Prevent copy-from from discarding armour values #54801

Merged
merged 3 commits into from
Jan 28, 2022

Conversation

dseguin
Copy link
Member

@dseguin dseguin commented Jan 26, 2022

Summary

None

Purpose of change

Fixes #53130.
There are really 2 issues:

  1. The "material_thickness", "environmental_protection" and "environmental_protection_with_filter" fields are discarded from the parent object when using copy-from.
  2. Attempting to use "proportional" or "relative" on the fields above causes an exception.

Describe the solution

  1. Store the values in the islot_armor struct so that a copied instance can reuse the parent's values.
  2. Bypass cata::optional when reading the values from JSON. The "proportional" and "relative" fields require the value type to support the *= and += operators, respectively.

Describe alternatives you've considered

#54681 used a workaround, but doesn't solve the underlying problem.

Testing

Added test case to verify behaviour for "copy-from", "relative" and "proportional".

Also tested in-game using Saicchi's example in the linked issue:

{
  "id": "xl_armor_chitin",
  "type": "ARMOR",
  "name": { "str": "XL chitinous armor" },
  "copy-from": "armor_chitin",
  "proportional": { "weight": 1.125, "volume": 1.13, "price": 1.25 },
  "extend": { "flags": [ "OVERSIZE" ] },
  "armor": [ { "encumbrance": 10, "coverage": 90, "covers": [ "torso", "leg_l", "leg_r" ] } ]
}
Before

not_fixed

After

fixed

Additional context

See #43144 for more context about "proportional" and "relative".

@dseguin dseguin force-pushed the fix_armour_copyfrom branch from 3d1cb1a to 9bb884a Compare January 26, 2022 08:46
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jan 26, 2022
@Maleclypse Maleclypse added <Bugfix> This is a fix for a bug (or closes open issue) Items / Item Actions / Item Qualities Items and how they work and interact labels Jan 26, 2022
@catdach
Copy link
Contributor

catdach commented Jan 26, 2022

A possibly related issue is that "encumbrance" also cannot be altered via "proportional" or "relative"; as the "armor_acidchitin"("biosilicified chitin armor") tries to do. Though I suspect it's because encumbrance is part of the "armor" field now? So it might be an entirely separate issue.

@dseguin
Copy link
Member Author

dseguin commented Jan 26, 2022

A possibly related issue is that "encumbrance" also cannot be altered via "proportional" or "relative"; as the "armor_acidchitin"("biosilicified chitin armor") tries to do. Though I suspect it's because encumbrance is part of the "armor" field now? So it might be an entirely separate issue.

That's a separate issue (well, multiple issues really):

  1. "proportional" and "relative" don't handle stuff in nested objects (i.e.: "armor": []). That would have to be handled as an elaborate special case.
  2. You can't copy-from individual "armor" objects since they don't exist as individual types, which is a requirement for "proportional" and "relative".

This fixes 2 things:
- Stores material_thickness, environmental_protection,
  and environmental_protection_with_filter inside islot_armor
  to properly inherit from the copied object.
- Workaround for cata::optional, which was causing
  "proportional" and "relative" to fail, because cata::optional
  does not implement operator*= and operator+= respectively.
On top of preventing exceptions when attempting to modify
cata::optional values, handle these fields so that they actually
modify the given fields:

- "material_thickness"
- "environmental_protection"
- "environmental_protection_with_filter"
@dseguin dseguin force-pushed the fix_armour_copyfrom branch from 9bb884a to fe2bfbc Compare January 27, 2022 06:00
Verifies that "copy-from", "relative" and "proportional" work as
expected for armour values.
@dseguin dseguin force-pushed the fix_armour_copyfrom branch from fe2bfbc to 5b376dc Compare January 27, 2022 06:52
@dseguin dseguin marked this pull request as ready for review January 27, 2022 07:35
@kevingranade kevingranade merged commit 5eb216d into CleverRaven:master Jan 28, 2022
@dseguin dseguin deleted the fix_armour_copyfrom branch January 28, 2022 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) Items / Item Actions / Item Qualities Items and how they work and interact json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

armor data resetting material_thickness and environmental_protection
4 participants