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 DeltaV syndicate objective "teach a lesson" #232

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Ermucat
Copy link

@Ermucat Ermucat commented Dec 11, 2024

About the PR

Ported DeltaV-Station/Delta-v#2184 from deltaV to harmony(Is ported the right word? I guess it is kinda stealing that might be frowned apon idk)

Why / Balance

Currently the Kill or Maroon objective is the only way of doing murder within your objectives, but with the limitation that the murdered one must be permanant killed. This system CAN be anti-fun for the victim and the murderer, I dont think it should be removed but providing a alternative I feel is needed

Technical details

Added new folder to Harmony/Locale/En-us
Added Teachalessoncomponent
added Teachalessonconditionsystem
added things to traitor objective ymls

Media

Yay.mp4

Requirements

Breaking changes

Changelog

🆑

  • add: Added new syndicate objective, Teach someone a lesson

@github-actions github-actions bot added the S: Needs Review Review is requested label Dec 11, 2024
Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

  • When porting Delta V or other fork content, please preserve the DeltaV namespace as _DeltaV.
  • Additionally, try to trace any changes to the feature after initial PR and implement them too.
  • Additionally, entities from base_objectives.yml and traitor.yml can be moved to the _Harmony and/or _DeltaV.

I will do a more thorough review once these points are addressed.

Thank you for working on this! Adding Admin input label while I am at it.

@FluffMe FluffMe added S: Awaiting Changes Reviewer requested changes S: Awaiting Admin Input Discussion by the admin team is required and removed S: Needs Review Review is requested labels Dec 11, 2024
@spanky-spanky
Copy link
Collaborator

There has been very recent admin discussion of wanting more diverse objectives so the admin consensus will probably be yes.

@Ermucat
Copy link
Author

Ermucat commented Dec 11, 2024

@FluffMe
Could you ellaborate on what preserving the namespace is?

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 11, 2024

@FluffMe
Could you ellaborate on what preserving the namespace is?

  • Original files are in DeltaV folders. They should be in _DeltaV folders on Harmony.
  • c# files have namespace declaration. These should also keep DeltaV in the namespace and they mimic the filepaths. If they are missing, they should be added

@Ermucat Ermucat requested a review from FluffMe December 11, 2024 23:31
@github-actions github-actions bot added S: Needs Review Review is requested and removed S: Awaiting Changes Reviewer requested changes labels Dec 11, 2024
Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

Namespaces are very wrong.
Can't review in detail from the phone, but I'll fix it in the next couple of days or on the weekend, unless you figure it out sooner.

Check for harmony C#-containing PRs for reference, if need be.

@FluffMe FluffMe added DO NOT MERGE and removed S: Needs Review Review is requested labels Dec 12, 2024
@TheCrimsonJupiter
Copy link

TheCrimsonJupiter commented Dec 12, 2024

Does this replace the current kill/maroon objectives or does it add a new kind of objective for traitors? If it replaces them like they do on DeltaV, I foresee a big rule conflict and general bad vibes.

There are many traitor tools and methods of killing that are designed to round remove, should this no longer ever be a requirement these sorts of tools will just be rulebreaks if they are used
"Minor antagonists... should not: Actively round remove people not listed as targets by you or another syndicate."
from Rule 5: Play Antagonists Responsibly. Should this feature REPLACE kill/maroon, like it does on DeltaV, then using any of these methods would be a rule violation and thats just bad game design to have features that conflict with our philosophy. Similar applies regardless of rules. Should somebody be murdered as a "teach a lesson" target and then fly off into space and be round removed, thats going to be as annoying and pissy as being killed as a non-target does now.

All of this doesn't really apply should it be added as a separate objective, for then discretion and use of tools can be scaled to the task at hand.

@Ermucat
Copy link
Author

Ermucat commented Dec 12, 2024

It does not replace kill and maroon, it is merely a diffreent alternative to add variety

@TheCrimsonJupiter
Copy link

It does not replace kill and maroon, it is merely a diffreent alternative to add variety

In that case all I wrote was just nonsense rambling. I'm a-okay with this as a new objective, but I think the name should be changed "Teach (name) a lesson" doesn't make any sense. When somebody wakes up from being dead they shouldn't be remembering anything that happened, so there is no lesson being taught here. If there was something with leaving a business card on a dead person so they find it when they wake up maybe that'd make more sense, but that'd take some extra programming stuff that I don't know if anybody is interested in. Something like "Kill them once" would be very basic and not very interesting, but would be less vague and nonsense and get the idea across a million times better.

@Ermucat
Copy link
Author

Ermucat commented Dec 12, 2024

Well we have cards in game, so thats up to player choice. And the name can be changed to be more descriptive thats just what DeltaV/SS13 had it as. A more vague objective allows players to come up with their own reason for why they murder this guy, I agree with that sentiment

@RotEmpress
Copy link

There has been very recent admin discussion of wanting more diverse objectives so the admin consensus will probably be yes.

+1
A welcomed change and adds more objective variety. I have no issues with this being added.

Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

First big bunch of required changes.

After you are done with this you should also implement DeltaV-Station/Delta-v#2412 and test everything properly. Milon's PR also properly sets the DeltaV namespaces.

Trace any other changes or PRs to these files that might be important.

Once this work is done, please test it thoroughly and make sure to keep as much of DeltaV code intact as possible. If you make any changes you must declare them per AGPL licensing. No renaming of the files where it's not needed, etc.

This should be a lot of work for someone with no C# experience, so thank you for tackling this. Feel free to ask questions on Discord if you need help. Experienced contributors should be able to help you.

Good luck!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong location for a component. Not sure why you renamed it too.
Should be Content.Server/_DeltaV/Objectives/Components/TeachLessonConditionComponent.cs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong location for a system. And ststem is not a word. Not sure why you renamed it too. Names of CS files should match the class name.
Should be Content.Server/_DeltaV/Objectives/Systems/TeachLessonConditionSystem.cs

Copy link
Collaborator

Choose a reason for hiding this comment

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

DeltaV content. Wrong placement and renamed for some reason.
Relocate to Resources/Locale/en-US/_DeltaV/objectives/conditions/teach-person.ftl

@@ -111,3 +111,4 @@
id: BaseCodeObjective
components:
- type: CodeCondition

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary change to upstream file. Please revert.

Comment on lines +1 to +15
# teach lesson
- type: entity
abstract: true
parent: BaseTargetObjective
id: BaseTeachLessonObjective
components:
- type: Objective
unique: false
icon:
sprite: Objects/Weapons/Melee/fireaxe.rsi
state: icon
- type: ObjectiveBlacklistRequirement
blacklist:
components:
- SocialObjective
Copy link
Collaborator

Choose a reason for hiding this comment

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

This prototype should be declared in traitor.yml with TeachLessonRandomPersonObjective instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

DeltaV content. Wrong location.
Should be Resources/Prototypes/_DeltaV/Objectives/traitor.yml

using Content.Shared.GameTicking;
using Content.Shared.Mind;
using Content.Shared.Objectives.Components;
using DeltaV.Server.Objectives.Components;
Copy link
Collaborator

@FluffMe FluffMe Dec 15, 2024

Choose a reason for hiding this comment

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

Not needed and wrong. Remove this line (Line 6)

using Content.Shared.Objectives.Components;
using DeltaV.Server.Objectives.Components;

namespace DeltaV.Content.Server.Objectives.Systems;
Copy link
Collaborator

@FluffMe FluffMe Dec 15, 2024

Choose a reason for hiding this comment

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

Suggested change
namespace DeltaV.Content.Server.Objectives.Systems;
namespace Content.Server.Objectives.Systems;

We'll just keep this simple and keep original DeltaV namespaces for this. Namespaces are further fixed by Milon in subsequent PR that you should implement too.

@FluffMe FluffMe changed the title Ports new syndiecate objective "teach a lesson" from https://github.com/DeltaV-Station/Delta-v/pull/2184 Port DeltaV syndicate objective "teach a lesson" Dec 15, 2024
@FluffMe FluffMe added the S: Awaiting Changes Reviewer requested changes label Dec 15, 2024
…t.cs

Co-authored-by: FluffMe <1780586+FluffMe@users.noreply.github.com>
Signed-off-by: Ermucat <190304574+Ermucat@users.noreply.github.com>
@FluffMe FluffMe marked this pull request as draft December 15, 2024 18:41
@FluffMe
Copy link
Collaborator

FluffMe commented Dec 15, 2024

Marked as draft to avoid constant test runs on Work In Progress. Undraft when it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE S: Awaiting Admin Input Discussion by the admin team is required S: Awaiting Changes Reviewer requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants