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

Expand generic factory to include relative and proportional support. #43144

Merged
merged 1 commit into from
Jul 3, 2021

Conversation

anothersimulacrum
Copy link
Member

@anothersimulacrum anothersimulacrum commented Aug 23, 2020

Summary

SUMMARY: Infrastructure "Allow using relative and proportional with generic_factory"

Purpose of change

In the hope of eventually removing the assign() functions, and expanding support for generic factory (and continuing to support existing use cases), generic_factory needs to support relative and proportional.

Also, some enums needed to be converted to enum classes so the compiler wouldn't get confused and try to use them with proportional.

Fixes #43117

Describe the solution

To do this, do some template vodoo.
Create two templated structs, and take advantage of SFINAE (yeah, I had to look that up) with these to determine which template function to use.
Three basically discriminate between whether or not the type can specify value proportionally or relatively.
For proportionally, this is whether or not the type can be multiplied by a float, and for relative, this is whether or not the type can use the += operator with itself.

Also, add some error checking for when a proportional value is inappropriately specified.

Huge thanks to jbytheway for helping me use template vodoo to accomplish this.

Testing

Game loads without error. Bio-chitin armor has acid protection.

@anothersimulacrum anothersimulacrum added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Aug 23, 2020
@jbytheway
Copy link
Contributor

Maybe do the conversion to enum class as a separate PR first to make this easier to review?

src/generic_factory.h Outdated Show resolved Hide resolved
src/generic_factory.h Show resolved Hide resolved
src/generic_factory.h Outdated Show resolved Hide resolved
src/generic_factory.h Outdated Show resolved Hide resolved
src/generic_factory.h Outdated Show resolved Hide resolved
@jbytheway
Copy link
Contributor

Based on the gcc 8 compiler error it looks like bool is supporting proportional (and it's probably supporting relative too), which you probably don't want. Might need to explicitly blacklist that, e.g. by explicitly specializing supports_relative<bool> etc.

src/generic_factory.h Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Oct 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Oct 7, 2020
@kevingranade kevingranade removed the stale Closed for lack of activity, but still valid. label Oct 7, 2020
@stale
Copy link

stale bot commented Nov 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added stale Closed for lack of activity, but still valid. and removed stale Closed for lack of activity, but still valid. labels Nov 7, 2020
@anothersimulacrum anothersimulacrum force-pushed the generic_aunt branch 2 times, most recently from d9a9c7c to e007687 Compare November 8, 2020 22:56
@anothersimulacrum
Copy link
Member Author

Fairly sure everything here is working and is in a good enough state for this to be merged. So I think this is just waiting on 0.F.

@Wishbringer
Copy link
Contributor

Wishbringer commented Nov 8, 2020

Should this have 0.F feature freeze tag?
This fixes ammo types that use copy-from and relative damage_type with armor_penetration.
Example ammo id "50_mk211" gets 0 penetration rather than +20 without this.
Seems like a fairly serious bug to have in a milestone release.

@anothersimulacrum
Copy link
Member Author

anothersimulacrum commented Nov 8, 2020

As the person who introduced that bug, as far as I can tell, it's a display issue. As long as the damage is loaded using assign(), which it is right now, relative/proportional work correctly for it. You can test this using 9mm against an armored zombie if you're interested (and editing values).
I do intend to look into that and fix it before 0.F, but it's not a loading issue as far as I can tell.

@stale
Copy link

stale bot commented Dec 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Dec 9, 2020
@anothersimulacrum anothersimulacrum removed the stale Closed for lack of activity, but still valid. label Dec 9, 2020
Salty-Panda added a commit to Salty-Panda/Cataclysm-DDA that referenced this pull request Apr 17, 2021
Copies entire entry, replace using copy-from once CleverRaven#43144 will get merged
Salty-Panda added a commit to Salty-Panda/Cataclysm-DDA that referenced this pull request May 3, 2021
Copies entire entry, replace using copy-from once CleverRaven#43144 will get merged
In the hope of eventually removing the assign() functions, and expanding
support for generic factory (and continuing to support existing use
cases), generic_factory needs to support relative and proportional.

To do this, do some template vodoo.
Create two templated structs, and take advantage of SFINAE (yeah, I had
to look that up) with these to determine which template function to use.
Three basically discriminate between whether or not the type can specify
value proportionally or relatively.
For proportionally, this is whether or not the type can be multiplied by
a float, and for relative, this is whether or not the type can use the
+= operator with itself.

Also, add some error checking for when a proportional value is
inappropriately specified.

Containers using the reader functions do not have proportional and
relative support, because I'm not sure of a use case where that ever
makes sense.

Huge thanks to jbytheway for helping me use template vodoo to accomplish
this.
@anothersimulacrum anothersimulacrum changed the title [CR] Expand generic factory to include relative and proportional support. Expand generic factory to include relative and proportional support. Jul 3, 2021
@kevingranade kevingranade merged commit add6f7d into CleverRaven:master Jul 3, 2021
@anothersimulacrum anothersimulacrum deleted the generic_aunt branch July 3, 2021 23:44
Salty-Panda added a commit to Salty-Panda/Cataclysm-DDA that referenced this pull request Aug 14, 2021
Copies entire entry, replace using copy-from once CleverRaven#43144 will get merged
ZhilkinSerg pushed a commit that referenced this pull request Aug 15, 2021
* Add option to raise the visor to motorcycle helmet

Copies entire entry, replace using copy-from once #43144 will get merged

* Add option to raise the visor to motorcycle helmet

* remove deprecated armor_portion_data

* Update data/json/items/armor/helmets.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Biosilicified) chitin armor doesn't actually protect from acid
4 participants