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

Add Multiple Tracking Options to the Pinpointer #418

Closed
wants to merge 8 commits into from

Conversation

Spatison
Copy link
Contributor

@Spatison Spatison commented May 23, 2024

Description

PR adds a system of multiple targets for one pinpointer.
Reworks the pinpointer for nuclear operatives, adding the ability to track a nuclear sindicat bomb and a syndicate shuttle along with the nuclear authentication disk.
Nuclear bomb in Shuttle Syndicate now always comes with its codes.


TODO

  • Add multiple Pinpointer

Media

https://youtu.be/YCcFzrtYp8E


Changelog

🆑

  • add: Multiple pinpointer
  • add: Syndicat nuclear bomb
  • fix: Sindicat to Syndicate
  • delete: Syndicat Nuclear Code
  • change: The station's nuclear code is now connected to any bomb on the same map

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: Map Changes any yml file in the Maps directories labels May 23, 2024
@DangerRevolution
Copy link
Contributor

Description

my first PR. I do not know what to write here.

What does this PR do?

@FoxxoTrystan
Copy link
Member

Can you edit this PR description and give more details?

What exactly is it? why is it needed, ect...

@DEATHB4DEFEAT
Copy link
Member

Media: https://drive.google.com/file/d/1UyXTHHVVUx7ZAvHG4_U6_ncnWWrn4FyE/view?usp=drivesdk

It would be preferable for both ease and history to directly embed it in the PR description.
If you can't reasonably compress it to fit 8MB I'd recommend putting it on YouTube instead of Google Drive so it's easier to keep longer and you get free views on your channel.
No matter what option you pick at least put media in the media section of the PR description.

@github-actions github-actions bot added the Status: Needs Review Someone please review this label May 23, 2024
@DangerRevolution DangerRevolution added Changes: YML Changes any yml files Priority: 4-Low Should be resolved at some point Size: 3-Medium For medium issues/PRs Status: Needs Cleanup Someone has to clean this before merging Type: Feature Creation of or significant changes to a feature labels May 23, 2024
Copy link
Member

@Pspritechologist Pspritechologist left a comment

Choose a reason for hiding this comment

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

Overall I'd like to see the solution made less hard-coded. Right now it requires modifying an enum to add new modes to a pinpointer- ideally a 'mode' would be a serializable object, allowing you to define the properties of that mode entirely in YAML.

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid modifying files that aren't relevant. You should be able to simply 'revert changes' on this file without affecting anything.

Copy link
Member

Choose a reason for hiding this comment

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

This is certainly not relevant to this pr :P

Copy link
Contributor

Choose a reason for hiding this comment

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

This.

/// Used for tracking the nuke bomb - isn't a tag for pinpointer purposes.
/// </summary>
[RegisterComponent, NetworkedComponent]
public sealed partial class NuclearBombSindicateComponent : Component
Copy link
Member

Choose a reason for hiding this comment

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

This isn't how you spell Syndicate.

Copy link
Contributor

Choose a reason for hiding this comment

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

This.

I assume this the closest English spelling of Синдикат

{
ALL,
STATION,
SINDICAT
Copy link
Member

Choose a reason for hiding this comment

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

This is even less how you spell Syndicate

Comment on lines 42 to 47
public enum NukesAvailable : byte
{
ALL,
STATION,
SINDICAT
}
Copy link
Member

Choose a reason for hiding this comment

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

The solution of having an Enum for explicit 'modes' of nuke tracking being 'All', 'Station', and 'Syndicate' is a very hardcoded solution to what should be entirely doable in YAML. I feel for this pr to make sense the Pinpointer itself just needs to be slightly reworked.


private void OnMultiplePinpointerStartup(EntityUid uid, MultiplePinpointerComponent multiple, ComponentStartup args)
{
if (EntityManager.TryGetComponent(uid, out PinpointerComponent? tool))
Copy link
Member

Choose a reason for hiding this comment

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

This should probably just be an EnsureComp rather than not doing anything if it isn't found.

Copy link
Member

Choose a reason for hiding this comment

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

The public methods here should have XML docs.

if (!Resolve(uid, ref multiple, ref pin))
return;

Dirty(uid, multiple);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're dirtying it right at the start?

Comment on lines 61 to 70
var modes = new string[multiple.Modes.Count];
var i = 0;

foreach (var tag in multiple.Modes)
{
modes[i] = tag;
i++;
}

var current = modes[multiple.CurrentEntry];
Copy link
Member

Choose a reason for hiding this comment

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

Am I crazy or does this not make any sense?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to be some unneeded changes in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

This.

@DangerRevolution DangerRevolution changed the title add Multiple Pinpointer Add multiple tracking options to the pinpointer May 24, 2024
Copy link
Contributor

@DangerRevolution DangerRevolution left a comment

Choose a reason for hiding this comment

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

I'm repeating a lot of what PSprite said but this is not a flexible system. The system, ideally, should track any entity with specific tags or components, defined in .yml.

As an example, I should be able to make a pinpointer that tracks the closest chair with a tag or component of "wooden".

I should be able to define in .yml what the pinpointer should track, is the point.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does nothing. Refer to above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This.

nuke.OriginStation != owningStation))
nuke.OriginMapGrid != (transform.MapID, transform.GridUid) ||
nuke.OriginStation != owningStation)) ||
(NukesAvailable == NukesAvailable.SINDICAT &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(NukesAvailable == NukesAvailable.SINDICAT &&
(NukesAvailable == NukesAvailable.SYNDICATE &&

Copy link
Contributor

Choose a reason for hiding this comment

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

This.

/// Used for tracking the nuke bomb - isn't a tag for pinpointer purposes.
/// </summary>
[RegisterComponent, NetworkedComponent]
public sealed partial class NuclearBombSindicateComponent : Component
Copy link
Contributor

Choose a reason for hiding this comment

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

This.

I assume this the closest English spelling of Синдикат

name: syndicate pinpointer
description: Produced specifically for nuclear operative missions, get that disk!
id: PinpointerLonolyOperative
suffix: LonolyOps
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this suffix name.

suffix: Sindicat
components:
- type: NukeCodePaper
NukesAvailable: Sindicat
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NukesAvailable: Sindicat
NukesAvailable: Syndicate

- type: entity
parent: NukeCodePaper
id: NukeCodePaperSindicat
suffix: Sindicat
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
suffix: Sindicat
suffix: Syndicate


- type: entity
parent: NukeCodePaper
id: NukeCodePaperSindicat
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id: NukeCodePaperSindicat
id: NukeCodePaperSyndicate

@@ -114,6 +114,26 @@
satchel: ClothingBackpackDuffelSyndicateOperative
duffelbag: ClothingBackpackDuffelSyndicateOperative

#Syndicate Lonoly Operative Outfit - Full Kit
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a full gear change from the regular operatives? To include the pinpointer?

@github-actions github-actions bot removed the Changes: YML Changes any yml files label May 24, 2024
@Spatison
Copy link
Contributor Author

So, how can i fix the checks?

@VMSolidus
Copy link
Member

So, how can i fix the checks?

I got it for you, the tests should run on your PR now. It just required a merge master to get a key update to make the tests work again after Microsoft broke them.

@DangerRevolution
Copy link
Contributor

Going to set this as draft; issues marked by PSprite are still not addressed

@DangerRevolution DangerRevolution marked this pull request as draft June 20, 2024 22:56
@SimpleStation14 SimpleStation14 changed the title Add multiple tracking options to the pinpointer Add Multiple Tracking Options to the Pinpointer Jun 21, 2024
Comment on lines 117 to 119
#Syndicate Lonoly Operative Outfit - include a special pinpointer
- type: startingGear
id: SyndicateLonolyOperativeGearFull

Choose a reason for hiding this comment

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

typo

components:
- type: Transform
pos: -2.5,-12.5
parent: 1
- proto: NukeCodePaper
- proto: NukeCodePaperStation

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done so that the nuclear bomb in Shuttle Syndicate is always provided with its own codes.

@@ -332,6 +332,18 @@
- id: PinpointerSyndicateNuclear
- id: DeathAcidifierImplanter

- type: entity
parent: ClothingBackpackDuffelSyndicateBundle
id: ClothingBackpackDuffelSyndicateLonolyOperative

Choose a reason for hiding this comment

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

typo

@@ -62,7 +62,7 @@
randomizeName: false
- type: NukeOperative
- type: Loadout
prototypes: [SyndicateOperativeGearFull]
prototypes: [SyndicateLonolyOperativeGearFull]

Choose a reason for hiding this comment

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

typo

Comment on lines 134 to 135
satchel: ClothingBackpackDuffelSyndicateLonolyOperative
duffelbag: ClothingBackpackDuffelSyndicateLonolyOperative

Choose a reason for hiding this comment

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

Do these exist?

@DangerRevolution
Copy link
Contributor

@Spatison will review this in 4 hours. This might need changes, not sure..

@github-actions github-actions bot added the Status: Merge Conflict FIX YOUR PR AAAGH label Jul 20, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DangerRevolution
Copy link
Contributor

@Spatison is this being worked on?

@Spatison
Copy link
Contributor Author

I am closing this project, I do not have the time and desire to work on it in the near future

@Spatison Spatison closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Changes: Map Changes any yml file in the Maps directories Priority: 4-Low Should be resolved at some point Size: 3-Medium For medium issues/PRs Status: Merge Conflict FIX YOUR PR AAAGH Status: Needs Cleanup Someone has to clean this before merging Status: Needs Review Someone please review this Type: Feature Creation of or significant changes to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants