Conversation
Terkala
requested review from
DrSmugleaf,
Princess-Cheeseballs,
crazybrain23 and
sowelipililimute
as code owners
PJBot
added
S: Untriaged
|
RSI Diff Bot; head commit 71e59b7 merging into 899c8d2 Resources/Textures/BloodCult/Clothing/cult_boots.rsi
Resources/Textures/BloodCult/Clothing/cult_hood.rsi
Resources/Textures/BloodCult/Clothing/cult_robes.rsi
Resources/Textures/BloodCult/Effects/culteyes.rsi
Resources/Textures/BloodCult/Effects/halo.rsi
Resources/Textures/BloodCult/Objects/Weapons/Melee/bloodcult_dagger_curved.rsi
Resources/Textures/BloodCult/Objects/Weapons/Melee/bloodcult_dagger_serrated.rsi
Resources/Textures/BloodCult/Objects/Weapons/Melee/bloodcult_dagger_straight.rsi
Resources/Textures/BloodCult/Sheets/glass.rsi
Resources/Textures/BloodCult/Sheets/metal.rsi
Resources/Textures/BloodCult/Sheets/plasteel.rsi
Resources/Textures/BloodCult/spells_cultist.rsi
Resources/Textures/Interface/Misc/job_icons.rsi
|
slarticodefast
left a comment
slarticodefast
left a comment
There was a problem hiding this comment.
Quick first-pass look, not a full review.
I would really recommend splitting this up further so it can be reviewed more easily.
Best start with only the conversion system, which should be unified with that of the revolutionaries.
Content.Client/BloodCult/Systems/BloodCultMeleePredictionSystem.cs
Outdated
Show resolved
Hide resolved
Content.Client/BloodCult/Systems/BloodCultMeleePredictionSystem.cs
Outdated
Show resolved
Hide resolved
Content.Client/BloodCult/Systems/BloodCultMeleePredictionSystem.cs
Outdated
Show resolved
Hide resolved
|
|
||
| [Serializable, NetSerializable] public sealed partial class DrawRuneDoAfterEvent : SimpleDoAfterEvent | ||
| { | ||
| [NonSerialized] public EntityUid CarverUid; |
PJBot
added
S: Awaiting Changes
|
Also for future reference, can you link or screenshot the permission to relicence from all the authors here on this PR? Or just ask them if they could comment here saying that they agree with it. |
|
Oh, and please add media to your PR description showing all the added features in-game. |
@kbarkevich (original author) via discord |
ThatGuyUSA
added
A: Roundflow/Antag
ThatGuyUSA
added
T: UI / UX Improvement
Moved a number of systems to content.shared instead of running on content.client and content.server. Fixed mute effect from the sanguine dream spell. Fixed an initialization issue with the bloodcult bleed type. Eliminated the ReagentAutoRefillSystem in favor of the existing system that did the same thing. Removed un-used code for runes (to be re-added later). Changed entity reference to the Entity<T> pattern. Added documentation. Changed how it selects beacon locations so it isn't hard-coded anymore.
|
Still working on addressing some of the code change requests and building out attempt-events for the melee weapon system |
…ked events as attempts
|
In case it's still needed here: I'm the original author, and I agree to the relicensing as long as I'm still given credit for my contributions. |
PJBot
added
S: Needs Review
|
|
Also removed holy water damage from the holy water medicine, since blood cultists can't take holy damage to begin with
|
I've gone over the comments now (took way less time than anticipated) and updated everything accordingly. Thank you for all the feedback. I've implemented almost everything suggested with a few exceptions, and commenting my reasoning behind not doing so. This should be good for a re-review now |
|
To be honest, I would follow slarti's advice and post the code in parts, as it is easier to review and refactor. I am eager to work on the blood cult and have made some progress in this direction. #42999 . The cult deserves good, non-hardcore code.😶 |
I'm extremely confused. That is exactly what I've done. This is part of the bloodcult code. They asked me to cut the original, 100% bloodcult PR into parts, so I cut it in half, in the only logical place I could think of to cut it in half. |
This is still a lot. I would divide it into, you know, equipment-constructs-buildings-abilities-runes-game rule. For example, you can look at the Xenoborg Pull Requests. |
The difference is that Xenoborgs was created while being PRed. Bloodcult is done, this code is all 100% finished and working. And there is massive cross-integration of those systems. Runes interact with the constructs. Constructs can interact with runes. Their equipment can interact with abilities and with the gamerule itself. So in order for me to split up those features without causing errors, I have to cut the code into pieces and intentionally break and change things just to make the review process easier. I have several pages of notes just on the features I broke to make this PR so I can go back and fix them if/when it ever gets accepted and I can PR the 2nd half. While it would be "nice" for me to put in this extra work to make it merged to Wizden, it's not really my focus. I was asked to move the code over to Wizden because people here were interested in it. My benefit from making this PR because the code review is actually helpful to the quality of the code, and because it makes maintenance of the bloodcult easier. It's in Funky's normal game mode rotation right now today, and has been for about a month with no issues. |
|
All requested changes have been addressed, save one or two small things that I left open discussion on. It's been months since anyone has commented, is this PR still being considered? |
It's still a feature maintainers would very much like to have in the game, but like I mentioned in my previous quick look this really needs to be split up and implemented step by step instead of one giant PR so that reviewing it is actually feasible. Like I mentioned above, the first step that is needed is probably the conversion mechanic, which should be a generic faction conversion system that is shared with revolutionaries and can be reused for other cults in the future. The second part is the spell system and BUI. This can be implemented without the actual spells, which then can be added one by one. You can also add an admin antag ctrl verb like we did for the changeling and wizard to allow in-game testing before a full release. If you address these points than we can see how we best move forward from there. |
slarticodefast
added
S: Awaiting Changes
|
While I do appreciate getting an update, as I stated previously that's a level of effort I'm simply not willing to put in here. Especially as the design document hasn't even been approved, meaning there's no guarantee any of this effort would result in anything. There's a significant gulf in the effort requires when Wizden asked me to port BloodCult and re implementing every feature in BloodCult from scratch. Especially since this whole PR process has been nothing but frustrating getting any kind of feedback up to now. It would take me an actual year to do this one PR at a time. Frankly speaking I could probably make two entirely new antagonists or major game systems from scratch with the level of time and effort you're demanding here. I welcome anyone from Wizden who wants to take the time to do this, as I will have a version fully up to date with NuBody and working in Funkystation's new codebase. |
6 participants
About the PR
This is the first half of adding the Blood Cult. I've cut the Blood Cult code in half, specifically this is the core functionality of the Blood Cult antagonist, the cult spells, and how the cult tracks progress. I've omitted all of the Blood Cult rune mechanics, Juggernauts, Shades, the Cult Rituals, and half of the progression mechanics (the ones that rely on rituals). I've commented out any hooks that reference these features, rather than delete them entirely. I've done my best to clean up dead code blocks from earlier implementations. Hopefully this trims the PR size down to small enough to be review-able (though it's still massive).
I've received permission from all of the original authors to release this as MIT code, and attributions for music and sound have been added. Art is added with attribution.
Why / Balance
Design Document explains why:
space-wizards/docs#591
Technical details
The core of the game mode is around the blood collection mechanic. The blood cult has to either convert people or spill their blood to progress, and the way this is tracked is both the conversion system (not included here) and the EdgeEssentiaSystem. Basically, when the cultists stab people with their daggers, it injects edge essentia. This triggers the metabolism system that makes them bleed unholy blood while metabolizing the chemical, and tracks the amount of bleeding they do while injected with edge essentia as cult progress. There is a counter that caps the gathered progress from any one player at 100, and it has a logic check to ensure a soul (player) is present. This is to prevent farming (via a chef making a monkey-cube stack and sacrificing all of them). Since the code for checking the cult progress via study-veil isn't in, use the admin command "cult_queryblood" to view the cult progress. Keep in mind phases 2 and 3 are calculated mid-round (based on 10% of all possible blood that is still remaining to be collected), so it'll show very small numbers during testing.
The cult dagger itself is special, and holds some Edge Essentia inside itself, which it injects on-hit. It also turns to ash if anyone not from the cult attempts to pick it up, preventing people from using it against cultists or to gather prints.
There is also a commune mechanic, which is an activateable ability that functions like a radio channel. Unlike a radio channel, it masks what you are saying with cult-gibberish so nobody can listen in. Which starts at a whisper, and eventually becomes loud once the cult has reached the later stages (not yet implemented in this partial port).
The placeholder for the Study Veil ability is there, but does not function in this version as the runes and cult progress have been disabled. So the ability is added but will not function.
Media
Requirements
Breaking changes
Changelog
No changelog, this is only a half-implementation.