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

Necromancer from Blackstone #536

Closed
wants to merge 7 commits into from
Closed

Conversation

Valkrae
Copy link
Contributor

@Valkrae Valkrae commented Aug 16, 2024

About The Pull Request

  • Adds Necromancer as a selectable role. They are disguised as baseline Mages/Sorceresses.
  • Necromancers have four spells to choose from.
  • Bone Chill - It heals undead mobs, and it damages non-undead. Non-undead mobs are also paralyzed for a time. The time has been halved from what the value was on Blackstone. This can and will be changed if it proves to be problematic.
  • Eyebite - Blinds the target for a small period, and floods the target with negative energy, dealing a significant amount of damage.
  • Raise Undead - Summons a skeleton simplemob. Currently, they are spawned with basic peasant equivalent gear and stats. They do not attack the Necromancer, or other deadites.
  • Ray of Sickness - Floods the target with toxins. Will eventually make you vomit, and be stunned.
  • The current only goal of the Necromancer is to survive until the end of the week.

Why It's Good For The Game

This PR is my attempt at a port and re-balancing of the Blackstone Necromancer. I admit that the "balance" has been minimal, but I'd like to see how it performs ingame through a testmerge and feedback, similar to how Assassin was handled. I've tested extensively on a localhost, and there are no extreme errors of which I am aware.

Pre-Merge Checklist

  • You tested this on a local server.
  • This code did not runtime during testing.
  • You documented all of your changes.

. = ..()
var/turf/T = get_turf(targets[1])
if(isopenturf(T))
new /mob/living/carbon/human/species/skeleton/npc/peasant/(T)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it eventually die or is it permanent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

permanent, its a simplemob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i mean, if you beat it to death, it'll be dead, if that's what you're asking. but no, there's no timer that annihilates it from existence after a set period of time.

@sanshoom
Copy link

Necromancers after I use Churn Undead

owner.current.cmode_music = 'sound/music/combat_necromancer.ogg'


/datum/antagonist/assassin/roundend_report()
Copy link
Contributor

Choose a reason for hiding this comment

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

Copypasting is bad.....at least remember to rename datums....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, im stupid!

fixes for stuff i forgot
@Valkrae
Copy link
Contributor Author

Valkrae commented Aug 16, 2024

added some misc. fixes that were mentioned in discord

H.mind.AddSpell(new /obj/effect/proc_holder/spell/invoked/projectile/sickness)
H.mind.AddSpell(new /obj/effect/proc_holder/spell/invoked/eyebite)
var/datum/antagonist/new_antag = new /datum/antagonist/necromancer() // Adds the necromancer antag label.
H.mind.add_antag_datum(new_antag)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you added the proc on after_spawn I think this would be unnecessary, as you just do ADD_TRAIT(H, TRAIT_NECROMANCER) here instead, and it would automatically assign it the necromancer antag datum on after_spawn after it checks for this perk, which...you forgot to assign here on his own DM....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, what you're saying is I should add the perk, and remove the var/datum/antagonist/new_antag = new /datum/antagonist/necromancer() ?

sorry, this is my first actual PR that has more than 2 things changed, so im a little slow on the uptick.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll explain it the best I can:

On DRIFTERZ.dm, on the after_spawn() proc for drifters, you added a check: If the drifter has the trait TRAIT_NECROMANCER, assign to them the Necromancer antag datum (turn them into a Necromancer antag). Like so:

if(!H.mind)
	if(HAS_TRAIT(H, TRAIT_NECROMANCER)) // if the mob is a necromancer, they will be given the necromancer antag (i want to enact pain on this antag)
		var/datum/antagonist/new_antag = new /datum/antagonist/necromancer()
		H.mind.add_antag_datum(new_antag)

Here, you forced the Necromancer antag datum on the proc that equips all skills and changes stats when choosing a necromancer, basically copypasting the above proc (and forgot to assign the trait TRAIT_NECROMANCER to them) so the trait is literally unused, and the proc on after_spawn() never succeeds.

You gotta pick the method to assign antag datum, one or the other. Mind that if you copypasted Assassin, this was made so that admins could turn people into Assassins mid-round as an antag (which would also be good for Necromancer to have)

This can lead to conflicts as the Necromancer antag datum would be applied twice in immediate succession, had you not forgotten to include the ADD_TRAIT(H, TRAIT_NECROMANCER) to this line I'm quoting.

So the TL;DR is, yes, please replace these two lines:

var/datum/antagonist/new_antag = new /datum/antagonist/necromancer() // Adds the necromancer antag label.
H.mind.add_antag_datum(new_antag)

With just:
ADD_TRAIT(H, TRAIT_NECROMANCER)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, ill go ahead and do that!

@Cre77
Copy link
Contributor

Cre77 commented Aug 21, 2024

how about adding friendly examine text, so that player zombies and skeletons (maybe even vampires?) know this is a necromancer at a glance, and vice versa? like what /datum/antagonist/zombie/examine_friendorfoe(datum/antagonist/examined_datum,mob/examiner,mob/examined does in zomble[sic].dm

@Stutternov
Copy link
Contributor

Was this tested so it can go live?

@Valkrae
Copy link
Contributor Author

Valkrae commented Aug 27, 2024

Was this tested so it can go live?

I've tested it extensively in SP, but what I need to do now is see how it functions balance-wise in the larger server environment.

Copy link
Contributor

@DovaKitty DovaKitty left a comment

Choose a reason for hiding this comment

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

Looks good for testmerge. I'd like to see player reviews on the balance of the spells ingame.

Copy link
Contributor

@DovaKitty DovaKitty left a comment

Choose a reason for hiding this comment

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

Should be TM'd. Will need to see balance of the added spells, especially eyebite, which can paralyze.

@ThePainkiller ThePainkiller added the Merge conflict This PR won't compile due to merge conflics label Sep 1, 2024
@ThePainkiller
Copy link
Contributor

Once merge conflicts are solved, we will TM to test it in live, really excited about this one

@YuiY1997
Copy link
Contributor

YuiY1997 commented Sep 7, 2024

Has anybody DMed whoever this is on discord to tell them to resolve conflicts?

@ThePainkiller
Copy link
Contributor

Being addressed by the coder team, Rebel will try to force push things here, or we might close it and one of us will re-open it up to date

@Stutternov
Copy link
Contributor

I'll see about force pushing this code tomorrow to get this working.

Copy link
Contributor

@BadAtThisGame302 BadAtThisGame302 left a comment

Choose a reason for hiding this comment

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

Two weeks ago last change, seems very stale and unworked on to me.... it's a big shame, really.

@NPC1314 NPC1314 added the Branch out of date Branch needs to update with main before merging label Sep 21, 2024
@ThePainkiller ThePainkiller added Needs work Further code polishing is required to make this PR acceptable. New features This PR introduces entirely new mechanics or features labels Sep 25, 2024
@ThePainkiller
Copy link
Contributor

Is this still being worked on or can we shelve it for stale?

@NPC1314
Copy link
Contributor

NPC1314 commented Oct 5, 2024

Staler than bread packed in your spare bag and forgotten after that one trip to the lake, only to be found the next summer.

@NPC1314 NPC1314 closed this Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Branch out of date Branch needs to update with main before merging Merge conflict This PR won't compile due to merge conflics Needs work Further code polishing is required to make this PR acceptable. New features This PR introduces entirely new mechanics or features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants