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

Bring Aftershock in BN up to line with DDA #262

Closed
wants to merge 8 commits into from
Closed

Bring Aftershock in BN up to line with DDA #262

wants to merge 8 commits into from

Conversation

Mom-Bun
Copy link
Contributor

@Mom-Bun Mom-Bun commented Dec 13, 2020

Summary

SUMMARY: Mods "Updating Aftershock"

Purpose of change

As one of the maintainers for Aftershock; it is my job to make sure Aftershock is up to date. So far I've been focusing on DDA, but BN as a game still has players, and Aftershock's future offers a unique gameplay loop different from the base game, and it would be the shame for this interesting mod to limited to DDA, and as BN and DDA drift further apart, its better to settle conflicts now instead of later, so updating in the future can be easier.

Describe the solution

I basically just cherrypicked PRs from DDA to BN, this is my first time ever doing something like this, so it is undoubtedly messy as hell, so please do look over this and make sure everything is fine.

Describe alternatives you've considered

Letting Aftershock be deprecated in BNs, and only focusing on updating Aftershock on DDA.

Additional context

I couldn't figure out how to cherry pick C++ reliant PRs, so I do need these two PRs included, as they will be used in the future of aftershock.
CleverRaven/Cataclysm-DDA#45686
CleverRaven/Cataclysm-DDA#45632

"armor_portion_data": [
{ "covers": [ "eyes" ], "coverage": 33, "encumbrance": 0 },
{ "covers": [ "head" ], "coverage": 100, "encumbrance": 2 }
],
Copy link
Member

Choose a reason for hiding this comment

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

BN doesn't have armor_portion_data. You could either change it to old version or wait for me to port portions.
But if there aren't many items that use it, changing to old would be preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How long will it take for you to port portions?
Aftershock's gameplay loop will focus on warmth a lot, so a lot of items will be using it.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, you'd remove this item for now and re-add it later, in a PR that will make a good use of portions.
This current PR is big and any dependency on other PRs it may have will hold it up.

I can't say exactly how long will it take to port it properly, but I could write a simple hack that will make it load (improperly) into the old format, so that jsons work, then later fix it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, give me time as this is on the backburner for now, as I do not want this to take away from Aftershock development time, but I'll get back to this and fix these issues when I can.

"//": "Survival kit for survival in terrestrial planets. List based on the gear found on lifeboats.",
"subtype": "collection",
"container-item": "afs_landfall_kit_2",
"on_overflow": "spill",
Copy link
Member

Choose a reason for hiding this comment

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

Not supported yet. Should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, what is a good alternative to do for this? as the survival kit is important for the stranded spacer run.

Copy link
Member

@Coolthulhu Coolthulhu Dec 13, 2020

Choose a reason for hiding this comment

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

It will need testing, but spill behavior may be the default one. I'll check.

EDIT: Looks like overflow AKA spill is the default in BN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, thank you!

"type": "item_group",
"//": "Survival kit for survival in terrestrial planets. List based on the gear found on lifeboats.",
"subtype": "collection",
"container-item": "afs_landfall_kit_2",
Copy link
Member

Choose a reason for hiding this comment

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

Will need testing to see if it works. Entries can have it for sure, but I'm not sure if whole groups can.

"material": [ "steel", "ceramic" ],
"techniques": [ "WBLOCK_1", "DEF_DISARM" ],
"volume": "1750 ml",
"longest_side": "140 cm",
Copy link
Member

Choose a reason for hiding this comment

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

"longest_side" won't be supported and needs to be removed.

@@ -71,7 +80,7 @@
"id": "afs_hardlight_longbow",
"type": "GUN",
"symbol": "(",
"looks_like": "longbow",
"looks_like": "recurbow",
Copy link
Member

Choose a reason for hiding this comment

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

"recurbow" was removed, so it should look like longbow.

@@ -206,7 +215,8 @@
"dispersion": 60,
"durability": 9,
"clip_size": 1,
"loudness": 300
"loudness": 300,
"pocket_data": [ { "pocket_type": "MAGAZINE", "ammo_restriction": { "120mm": 1 } } ]
Copy link
Member

Choose a reason for hiding this comment

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

"pocket_data" won't be supported.

@@ -34,7 +99,7 @@
},
{
"type": "profession",
"ident": "afs_atomic_pitchman",
"id": "afs_atomic_pitchman",
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this needs to be "ident" for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know that was part of BN, I'll fix any mentions of id then.

Copy link
Member

Choose a reason for hiding this comment

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

Some old idents are preserved as ident instead of id, yes. The PR that finished converting the last remaining cases of ident came about after the version BN updated itself to before splitting off again, and the issue of "everyone needs to change their idents to ids" was likely why the change hadn't been made until this year.

@Mom-Bun
Copy link
Contributor Author

Mom-Bun commented Dec 15, 2020

I've been taking a glance at this whenever I get time, and talking to Candlebury about this; I'm realizing that the code differences are very significant for BN, and trying to port it over, while possible, is difficult.

Making an overhaul mod is difficult, and I want to make sure that my team and I design a good mod, not only to make something fun for people to enjoy, but to show the possibility of what you can do with Cataclysm, how you could push it and make something unique with it, and I fear that I cannot do that with my attention split constantly trying to juggle code differences.

I apologize that I won't be able to finish this PR, as I very much would want to see Aftershock in BN, but I don't want to risk disappointing Maleclypse or Candlebury, or even delivering a subpar experience.

If someone else comes to try porting it, feel free to notify me and I can help give pointers and advice on Aftershock.

@Mom-Bun Mom-Bun closed this Dec 15, 2020
@chaosvolt
Copy link
Member

chaosvolt commented Dec 15, 2020

It's mostly just a matter of keeping an eye on what features differ, and deciding which features are so vital that they need to be ported over to BN, Pocket data stuff for example is easy but tedious to fix. Relic and magic stuff is harder as it'll depend on whether or not an artifact effect can serve the same purpose or not.

I can try to take a look at it if you'd like.

@Mom-Bun
Copy link
Contributor Author

Mom-Bun commented Dec 15, 2020

It's mostly just a matter of keeping an eye on what features differ, and deciding which features are so vital that they need to be ported over to BN, Pocket data stuff for example is easy but tedious to fix. Relic and magic stuff is harder as it'll depend on whether or not an artifact effect can serve the same purpose or not.

I can try to take a look at it if you'd like.

I'd recommend you try to later once content freeze hits, as new content will stop dropping there so it'll be easier, but if you wish I would greatly appreciate it, I can answer any questions that you have if you want.

@chaosvolt
Copy link
Member

Suppose that'd be a good time for it too. Just looking at the PR, it still might be possible to see how well the items balance out with a single warmth value. Actually, the only example I see here is the communications cap, and I see down sane reason why it should have 33% eye coverage if it includes augmented reality glasses stuff (which would cover the eyes, and basically all other glasses items that I can recall have much higher coverage). It's not affecting warmth so it won't have any bearing on that gameplay loop.

Pocket data can be just straight-up deleted for guns and tools, and only needs a real change when it's used for:

  1. Clothing container pockets (which would just be using a storage value equal to the desired volume).
  2. Liquid containers with would use container_data instead.

Likewise longest_side would be deletable without much hassle since it doesn't replace any system retained in BN and activity_level in recipes can simply be removed. Container-group can probably be done via basically doing it the other way around, citing a contents-group on whatever it's supposed to spawn in, or making them work like first aid kits where the use action uncrafts them, depending on if what the containing item is.

The relic stuff I can see is fairly recent, the "items can add more damage in melee" feature I'd yet to tinker with porting. Looks like it's only used for the lightsaber. Rather obviously, changing the cut damage is trivial and using the ITEM_DAMAGE_CUT on an ACTIVE enchantment is redundant, but the fire damage would only have a few less-than-ideal alternatives:

  1. Using the flag flaming weapons use to deal some bonus fire damage, which isn't ideal since the bonus damage is fixed.
  2. Using an on-hit-enemy enchantment, which isn't ideal because it's coded weird and will trigger even if you miss. That is a major flaw with the code in both the BN and DDA versions and I don't yet know how to fix it, but if I can fix it to only trigger on actual hits it'd make weapon damage enchantments completely redundant.
  3. Adding a weapon technique to it that deals fire damage. While an example can be found in Arcana (https://github.com/chaosvolt/cdda-arcana-mod/blob/master/Arcana_BN/techniques.json#L85), that's used in a crit technique and therefore assumes it's not causing a damage spike with every hit (it was used as a less-weird replacement for the VORPAL technique, to make it ping more often, deal less damage, and deal an exotic damage type). Making it trigger every normal attack would be doable, except if it's dropped into any martial arts then martial arts techniques will override the effects of the weapon technique whenever valid.

Side note regarding the autoclave and other furniture overrides, I'm fairly certain copy-from overriding works for that, but that's not something on your end, just something those working on the DDA end might want to look into since it'll make things easier on them.

@Mom-Bun Mom-Bun deleted the aftershockup branch May 31, 2021 11:16
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.

7 participants