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

Port over mutation additions/changes from DDA #1298

Merged
merged 6 commits into from
Feb 19, 2022

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented Jan 31, 2022

Summary

SUMMARY: Content "Port over new mutations and related changes from DDA"

Purpose of change

Decided to port over a few easy mutations from DDA, basic stuff we don't need transforming mutations for.

Describe the solution

  1. Ported over Bovine Bulk and Bull Roids mutations from DDA, minus the tiny token general-movecost change and with the HP modifier swapped for bonus HP instead.
  2. Ported over change making Deft a Feline trait.
  3. Ported over change making Weak Scent affect Smelly.
  4. Ported over Light Bones mutations being marked as mixed effect.
  5. Ported over Patchwork Skin and Thickened Patchwork Skin from DDA, with a buff per feedback.
  6. Fixed Ink Glands not have string name.
  7. Ported over Bulging Reservoir from DDA.
  8. Ported over change making the Large series Chimera mutations.
  9. Ported over fix making Mouth Flaps stack with traits it evolves into.
  10. Ported over Persistence Hunter and Extreme Persistence Hunter from DDA, with point cost adjustments per feedback, and with them no longer affecting metabolism traits.
  11. Ported over Fast Reflexes and Feline Flexibility traits from DDA, with the latter having adjusted bonuses.
  12. Ported Strong Legs over from DDA, with proposed tweaks to bonuses.
  13. Misc: ported over some typo fixes and description tweaks from DDA. Most of this is just better explanations for mutation effects, with some spelling fixes and better punctuation.

Describe alternatives you've considered

  1. The effects of the Jekyl and Hyde mutation could technically be mimiced with an active mutation enchantment.
  2. Combat Adaption could likewise be mocked up via active mutation enchantments.

Testing

Tested affected file for syntax and load errors.

Additional context

Sources:

Honestly, the only trait I was especially needing to port over for something else I was messing with was Fast Reflexes, but the various improvements to Feline, Lupine, Chimera, and Bovine all seemed good to grab.

@@ -1269,6 +1294,19 @@
"cancels": [ "BIRD_EYE", "LIZ_EYE", "FEL_EYE", "URSINE_EYE", "COMPOUND_EYES", "ELFAEYES" ],
"category": [ "CEPHALOPOD" ]
},
{
"type": "mutation",
"id": "MANSBESTFRIEND",
Copy link
Member

Choose a reason for hiding this comment

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

Kinda bloaty, just social modifiers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Lupine also gets the Pred series, fair. There's a later mutation in DDA that's supposed to syncretize the two to fit a "pack hunter" theme, but I left it behind as I assume it has code changes of some sort.

"types": [ "SKIN" ],
"prereqs": [ "FELINE_FUR" ],
"category": [ "FELINE" ]
"category": [ "FELINE" ],
"armor": [ { "parts": [ "head", "mouth" ], "bash": 1 } ]
Copy link
Member

Choose a reason for hiding this comment

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

So minor it's not worth adding. Certainly not worth that 1 point.

"type": "mutation",
"id": "PATCHSKIN1",
"name": { "str": "Patchwork Skin" },
"points": 2,
Copy link
Member

Choose a reason for hiding this comment

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

1 at most

"type": "mutation",
"id": "PATCHSKIN2",
"name": { "str": "Thickened Patchwork Skin" },
"points": 4,
Copy link
Member

Choose a reason for hiding this comment

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

It's below "good for 1 point". Thick skin is better. At 4 points, it's utter trash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can either buff it or lower point cost.

"visibility": 1,
"description": "Your legs have gained muscle mass to propel you across any terrain.",
"prereqs": [ "PADDED_FEET" ],
"movecost_modifier": 0.88,
Copy link
Member

Choose a reason for hiding this comment

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

Weird modifier. What's the final one, when padded feet are included?
Also, the name sounds more like a bonus to weight capacity.

Copy link
Member Author

@chaosvolt chaosvolt Jan 31, 2022

Choose a reason for hiding this comment

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

Seems like a weird trait overall, want me to remove it or just change the bonuses?

"changes_to": [ "PERSISTENCE_HUNTER2" ],
"category": [ "LUPINE" ],
"fatigue_modifier": -0.1,
"stamina_regen_modifier": 0.1
Copy link
Member

Choose a reason for hiding this comment

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

How does it compare to less sleep and that +stamina trait? I'm pretty sure +stamina trait affects regen as well.

Copy link
Member Author

@chaosvolt chaosvolt Jan 31, 2022

Choose a reason for hiding this comment

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

It's 2/3 the bonus of Less Sleep, and gives a stamina regen bonus instead of max stamina. So it'd probably be better to remove the metabolism type given the modifers would otherwise clash with multiple traits that do other things, or of course can buff it.

If we keep it like this but without halting metabolism traits, it can probably be lowered to 1 point.

EDIT: Oh derp, it's already at 1 point and not set to clash with metabolism traits anyway, duh.

"type": "mutation",
"id": "PERSISTENCE_HUNTER2",
"name": { "str": "Extreme Persistence Hunter" },
"points": 10,
Copy link
Member

Choose a reason for hiding this comment

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

lmao that cost

"purifiable": false,
"threshreq": [ "THRESH_FELINE" ],
"category": [ "FELINE" ],
"movecost_obstacle_modifier": 0.5
Copy link
Member

Choose a reason for hiding this comment

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

How does it compare with parkour? Also, make sure this field is implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fucking lel, it looks like it's literally the same bonus as Parkour Expert but minus whatever weird hardcode stuff that trait has.

},
{
"type": "mutation",
"id": "HATES_WATER",
Copy link
Member

Choose a reason for hiding this comment

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

Bloat

We'll be getting rid of custom water protection one day. It'll be simplified to just flags like "immune to water morale" and "+morale from wet".

"cut": 2
}
],
"armor": [ { "parts": "ALL", "physical": 5 } ],
Copy link
Member

Choose a reason for hiding this comment

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

5 phys is better than 2, but the point cost is still 4, while the mutation is closer to Tough Skin (1pt) times two.

Copy link
Member Author

Choose a reason for hiding this comment

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

2 fits for point cost then.

"prereqs": [ "PADDED_FEET" ],
"movecost_modifier": 0.88,
"movecost_modifier": 0.8,
Copy link
Member

Choose a reason for hiding this comment

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

How does it stack with padded feet? What's the move cost of Strong Legs + Padded Feet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the code, Padded Feet is 0.9 movecost when you're barefoot, so combined it'd be 0.72 movecost. It'd be 0.792 if using the DDA version's old 0.88 modifier. If we want a total of 0.75 when barefoot then Strong Legs would need to be 0.8333. Or, we'd get 0.81 total if this was 0.9 as well.

Copy link
Member

Choose a reason for hiding this comment

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

It's just 1 point and Padded only works when your feet are not covered. Ignore the "you can walk barefoot with no penalty" thing, it's negligible (and I want to get rid of barefoot penalty).
So Strong Legs should only give enough bonus to bring you to 0.8 when combined with barefoot Padded.

"visibility": 4,
"ugliness": 3,
"description": "You develop a reservoir of fluids that bulges out of your abdomen. Your body has adapted to containing extra liquids in event of shortfalls.",
"thirst_modifier": 0.75,
Copy link
Member

Choose a reason for hiding this comment

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

Should probably clash with more thirst mutations.

@@ -637,6 +661,7 @@
"description": "Your scent is quite weak. Animals that track your scent will do so with more difficulty.",
"starting_trait": true,
"category": [ "ALPHA" ],
"cancels": [ "SMELLY2" ],
Copy link
Member

Choose a reason for hiding this comment

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

No SMELLY1 cancelling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, the cancellation here is redundant because it has a mutation type, doh.

"changes_to": [ "BOVINE_BULK2" ],
"category": [ "CATTLE" ],
"movecost_swim_modifier": 1.3,
"hp_adjustment": 15
Copy link
Member

Choose a reason for hiding this comment

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

How does it compare to Tough and the like? How does it stack with them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I was talked into using hp_adjustment instead of either modifier, then a base 84 HP character with TAAANK will go from 117.6 HP (40% modifier) to 132.6 HP. A strength 8+2 character (90 HP, Very Strong is the highest +Str trait Bovine can get) will go from 126 HP to 141 HP. That's assuming hp_adjustment is made after multipliers, if it's the other way around the values will be a 6-8 points higher.

So for a bovine character of average base strength but boosted by Very Strong, +15 HP is a bit over half what they can expect to get out of TAAANK, while +30 HP from the upgrade is about on par with said trait.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed it's not post-thresh. Should be renamed not to be about bulls.

The "roids" on should probably be renamed to something like "growth hormones" and mention that although your frame isn't larger, you've got a very robust build.

Seems like both mutations are missing a weight adjustment. Was it also absent in DDA or just not present in BN? Should probably be ported, since it's very clearly implying you're built like an APC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think BN has a mechanic for JSONized messing with character weight, and I'm not sure DDA does either. If it does have anything other than hardcoded "more weight for the size+ traits" then it DEFINITELY wasn't implemented in the DDA version as it was ported over.

@Coolthulhu Coolthulhu self-assigned this Feb 11, 2022
@kevingranade
Copy link
Contributor

You are removing credit from the commits you are copying, you need to either cherry-pick with arguments that preserve the author field, or use the commit --author parameter to commit as the original author.

@chaosvolt
Copy link
Member Author

I've tried and failed to get cherry-pick working, nevermind that 90% of commits I'd be able to port with it need heavy work to fit with BN, whether it be due to code drift or (just as often) fixing problems with DDA's implementation.

This PR in particular is rife with cases where DDA's version of the mutations in question need heavy changes to be worth bothering with, and that's after I restricted myself to only mutations that don't require DDA-specific things like transforming mutations or EoC. I'd also need help hunting down exactly which PRs added these, since at least then links to the source PRs can be posted in the PR OP.

In that situation the only viable option, if we want to port over DDA changes at all, would be to copy-paste things over the hard way and spend the inevitable time fixing and bugtesting.

@kevingranade
Copy link
Contributor

This is not a suggestion that you can argue your way out of, this is a license requirement.
If you can't do it right, you don't do it.

@chaosvolt
Copy link
Member Author

If you want to play this game, your project has several PRs that take from BN with proper attribution not preserved by commit authorship:

  1. https://github.com/CleverRaven/Cataclysm-DDA/pull/51341/commits
  2. https://github.com/CleverRaven/Cataclysm-DDA/pull/52184/commits
  3. https://github.com/CleverRaven/Cataclysm-DDA/pull/55161/commits

For your reference, here is an example of a PR with cherry-picking done properly: https://github.com/CleverRaven/Cataclysm-DDA/pull/53971/commits

I can provide links to the source PRs or commits in the PR OP, and given the circumstances I feel that would fall under giving appropriate credit, just as the PR summary qualifies as as indicating changes that were made:

Attribution — You must give appropriate credit, provide a link to the license, and indicate if changes were made. You may do so in any reasonable manner, but not in any way that suggests the licensor endorses you or your use. 

But if you're going to insist that proper cherry-picking is the only correct way to attribute authorship in a reasonable manner, you need to revert the first three PRs listed, and re-do them with proper attribution.

@kevingranade
Copy link
Contributor

I'm asking for attribution and outlining some options for providing it, if you're done making excuses and invalid counter-accusations, we can talk about resolving the issue.

Yes I think credit and sourcing in the PR description is a reasonable option.

@chaosvolt
Copy link
Member Author

Yes I think credit and sourcing in the PR description is a reasonable option.

Done, scoured the commit history for the file for what should be every PR to link to, but do inform me if I missed any.

@Coolthulhu
Copy link
Member

I'm gonna side with Kevin here.
Attributions are important when dealing with Open Source licenses. Authors usually do not get paid, so the least that should be done is linking to the source where possible. cherry-pick is better, cherry-pick+link is best.

@cyborg29x
Copy link
Contributor

cyborg29x commented Feb 15, 2022

In that case, surely it would be equally important to at least also attribute the proper credits to the PRs that the Dark Days Ahead project ported over from Bright Nights?

For example, the three PRs mentioned by @chaosvolt, which i will repost the links to, do not have these credits:

  1. https://github.com/CleverRaven/Cataclysm-DDA/pull/51341/commits
  2. https://github.com/CleverRaven/Cataclysm-DDA/pull/52184/commits
  3. https://github.com/CleverRaven/Cataclysm-DDA/pull/55161/commits

@chaosvolt
Copy link
Member Author

Eh, all of them at least provide a link to the source PR, so.

@Coolthulhu Coolthulhu merged commit 13bcbc7 into cataclysmbnteam:upload Feb 19, 2022
@chaosvolt chaosvolt deleted the port-over-some-mutations branch February 19, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants