-
Notifications
You must be signed in to change notification settings - Fork 276
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
feat(content,port): More NPCs
#3917
Conversation
The Autofix app has automatically formatted this Pull Request. If you edit your PR on web UI, you can ignore this message.
If you don't do this, your following work will be based on the old commit, and cause MERGE CONFLICT. |
More NPCs
More NPCs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the port. however, these commits are ported from DDA and thus must be attributed accordingly. could you add attribution to individual commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've left in DDA exclusive items in the dialogue. lc_steel_lump mc_steel_lump and hc_steel_lump are not in BN and are not candidates to port as those are different carbon grades of steel. I am also unsure if we even have muriatic acid in the game. You'll need to playtest the changes and remove references to things that don't exist.
I also may have missed it but I didn't see a blacksmith NPC in this pull that is referenced in the scrapyard questline, we need to either port that blacksmith or remove that quest for now.
I give my overall general approval of the npc's, quest, and dialogue once any JSON issues are ironed out. Asking thoughts on the NPC's from @KheirFerrum as the lore re-writer.
@scarf005 The tests won't account for item ID's we're missing from DDA. |
@chaosvolt could you review the changes made since my last review (from 2 weeks ago)? |
"type": "mapgen", | ||
"method": "json", | ||
"om_terrain": [ | ||
[ "apartments_con_tower_113", "apartments_con_tower_013" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended that this be basically identical to the mapgen entry above except for placing the NPC and related zones, by the way? I guess there's no real sane or easy way to shift the ownership and zone placement such that it only triggers if the NPC spawns, though nested chunk or mapgen update stuff could be done to pare this down into just being a fixed chance of spawning in the baseline variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed some easy fixes but moving the epilogues to the existing set of epilogues (honestly with how much other stuff this PR added related to him was redundant I'm surprised he doesn't already HAVE epilogues) will be slightly more complicated than what chance suggestions can do.
Co-authored-by: Chaosvolt <chaosvolt@users.noreply.github.com>
Co-authored-by: Chaosvolt <chaosvolt@users.noreply.github.com>
Co-authored-by: Chaosvolt <chaosvolt@users.noreply.github.com>
Co-authored-by: Chaosvolt <chaosvolt@users.noreply.github.com>
added Mr. Lapin's faction
added Mr. Lapin's faction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't see anything else that stands out as odd so think it's all good, but can let Scarf take a last-minute look at it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I highly trust CV
could merge once the tests pass.
}, | ||
{ | ||
"text": "[40$] I'll buy some aspirin.", | ||
"effect": [ { "u_buy_item": "aspirin", "container": "bottle_plastic_pill_prescription", "cost": 4000, "count": 20 } ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the problem. Sadly noone checked the container.
Purpose of change
This pull request imports several different NPCs. This will make the post-cataclysmic world feel more alive.
Describe the solution
Imports the three following dda prs
CleverRaven/Cataclysm-DDA#36287 by MaleclypseCleverRaven/Cataclysm-DDA#57483 by MNG-cataclysm
CleverRaven/Cataclysm-DDA#62134 by MNG-cataclysm
Describe alternatives you've considered
This could work as an standalone mod, but it works better integrated into the vanilla experience.
Testing
Spawned the various maps and verifed that NPCs dialogue options and missions work
Additional context