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

NVG (Port From _LostParadise) #1128

Closed
wants to merge 13 commits into from
Closed

Conversation

BL02DL
Copy link

@BL02DL BL02DL commented Oct 22, 2024

Description

The influence on the game is manifested in the fact that these are inherently improved visors of various departments with the ability to see in the dark, this is also necessary for the future cult of Narsi.

As I have already said, this night vision device is necessary in the code plan for the implementation of one of the objects of the cult


TODO

This Pull Request is still in progress.

Changelog

🆑 BL02DL

  • add: Added NVG!

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: YML Changes any yml files Changes: Sprite Changes any png or json in an RSI labels Oct 22, 2024
@SimpleStation14 SimpleStation14 changed the title NVG (Port from _LostParadise) NVG (Port From _LostParadise) Oct 22, 2024
Copy link
Contributor

github-actions bot commented Oct 22, 2024

RSI Diff Bot; head commit 6161b11 merging into 9430b90
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/Clothing/Eyes/NVG/diagnosticnightvision.rsi

State Old New Status
equipped-EYES Added
icon-on Added
icon Added
inhand-left Added
inhand-right Added
on-equipped-EYES Added
on-inhand-left Added
on-inhand-right Added

Resources/Textures/Clothing/Eyes/NVG/mednightvision.rsi

State Old New Status
equipped-EYES Added
icon-on Added
icon Added
inhand-left Added
inhand-right Added
on-equipped-EYES Added
on-inhand-left Added
on-inhand-right Added

Resources/Textures/Clothing/Eyes/NVG/nightvision.rsi

State Old New Status
equipped-EYES Added
icon-on Added
icon Added
inhand-left Added
inhand-right Added
on-equipped-EYES Added
on-inhand-left Added
on-inhand-right Added
toggle-off Added
toggle-on Added

Resources/Textures/Clothing/Eyes/NVG/secnightvision.rsi

State Old New Status
equipped-EYES Added
icon-on Added
icon Added
inhand-left Added
inhand-right Added
on-equipped-EYES Added
on-inhand-left Added
on-inhand-right Added

Edit: diff updated after 6161b11

@Eagle-0
Copy link
Contributor

Eagle-0 commented Oct 22, 2024

I would wait and port NVG's from WD instead, they have thermals too

@BL02DL
Copy link
Author

BL02DL commented Oct 22, 2024

@Eagle-0 They were going to port their old version, which would be worse than mine. There is no toggle button and Alert. There is no color setting.

@FoxxoTrystan FoxxoTrystan requested review from a team, VMSolidus, FoxxoTrystan, DEATHB4DEFEAT, Peptide90, Pspritechologist and OldDanceJacket and removed request for a team October 22, 2024 15:28
@github-actions github-actions bot added the Status: Needs Review Someone please review this label Oct 22, 2024
@FoxxoTrystan FoxxoTrystan added Size: 3-Medium For medium issues/PRs Holy Shit Priority: 3-Medium Needs to be resolved at some point Type: Feature Creation of or significant changes to a feature Type: Port Brings something to here from another codebase Status: Needs Review Someone please review this and removed Status: Needs Review Someone please review this labels Oct 22, 2024
Copy link
Contributor

@FoxxoTrystan FoxxoTrystan left a comment

Choose a reason for hiding this comment

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

All of the code is simple and good, good job.

The only changes i will request is to REMOVE all additional namespaces, we do not need "_LostParadise", remember we are an upstream not a downstream and those does not help us, please move it.

Russian FTL seem be missing?

What does "LPP" Means? i seen it everywhere and looks wired.

Some items names/copyright details need to be better.

Minor code suggestion but again, good job!

Content.Client/IoC/ClientContentIoC.cs Outdated Show resolved Hide resolved
Content.Client/IoC/ClientContentIoC.cs Outdated Show resolved Hide resolved
Content.Server/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Resources/Prototypes/_LostParadise/Shaders/shaders.yml Outdated Show resolved Hide resolved
@FoxxoTrystan
Copy link
Contributor

Can you also provide a "Media", how the nightvsion looks like when on? at least the default.

@FoxxoTrystan FoxxoTrystan removed the Status: Needs Review Someone please review this label Oct 22, 2024
@FoxxoTrystan FoxxoTrystan added the Status: Awaiting Changes Do not merge due to requested changes label Oct 22, 2024
Copy link
Contributor

@Remuchi Remuchi left a comment

Choose a reason for hiding this comment

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

Haven't seen it in the game yet but what is the point of NVG alert? You can clearly understand that you have NVG enabled by the look of your... welp... game screen? IMO at the moment it just looks like excessive information and alert bloat (which we already have too much).
Maybe you should at least make it able to toggle NVG on/off. But I'd rather remove it.

Content.Client/IoC/ClientContentIoC.cs Outdated Show resolved Hide resolved
Content.Client/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Client/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Client/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Client/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Server/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Server/_LostParadise/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
@BL02DL
Copy link
Author

BL02DL commented Oct 22, 2024

@FoxxoTrystan Hi. Unfortunately, I can't attach any video to demonstrate NVG's work, I can only try screenshots due to the fact that I didn't do it on my work computer. And it looks like I got the translation file mixed up..

@github-actions github-actions bot added Status: Needs Review Someone please review this and removed Status: Awaiting Changes Do not merge due to requested changes labels Oct 22, 2024
Copy link
Member

@VMSolidus VMSolidus left a comment

Choose a reason for hiding this comment

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

This is only a cursory check. The only thing that stood out while I was on the github web was the use of magic strings hardcoding it to only work on the Eyes slot.

Content.Shared/Clothing/NightVisionComponent.cs Outdated Show resolved Hide resolved
Content.Server/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Server/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Server/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Client/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
Content.Client/Clothing/NightVisionSystem.cs Outdated Show resolved Hide resolved
{
var playerUid = EntityUid.Parse(_playerManager.LocalPlayer?.ControlledEntity.ToString());
var slot = _entityManager.GetComponent<InventorySlotsComponent>(playerUid);
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision);
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision);

Oh this is hardcoded in the "eyes" slot here too. This is incredibly cursed. You shouldn't need a "magic string" here to check only the eyes slot. What if I wanted to make a mech that gives you night vision, or a hardsuit helmet with built-in nightvision?

Copy link
Author

Choose a reason for hiding this comment

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

@VMSolidus What i need to do to fix this?

Copy link
Author

Choose a reason for hiding this comment

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

@VMSolidus Please, tell me what i need to do..

Copy link
Member

Choose a reason for hiding this comment

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

Refer to what my maintainers are saying

Content.Shared/Clothing/NightVisionComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clothing/SharedNightVisionSystem.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Remuchi Remuchi left a comment

Choose a reason for hiding this comment

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

Mostly fine. After resolving Raislin review and my commentaries can be merged.

Comment on lines +100 to +109
- type: entity
id: ActionBaseToggleNightVision
name: Toggle NVG
description: Toggles the NVG on and off.
noSpawn: true
components:
- type: InstantAction
itemIconStyle: NoItem
event: !type:ToggleNightVisionEvent

Copy link
Contributor

Choose a reason for hiding this comment

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

I really see no reason for it to exist. It doesn't have anything abstract or generic to be separated in abstract entity nor it's used somewhere in the code. It can be removed entirely.

.desc = { ent-ActionBaseToggleNightVision }
ent-ClothingEyesNVG = night vision goggles
.desc = Now you can see in the dark! It has the label "BL CORP technology".
toggle-nightvision-verb-get-data-text = Toggle Night Vision Goggles
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have it placed after/before all of the ent fields for the ease of reading.

.desc = { ent-ActionBaseToggleNightVision }
ent-ClothingEyesNVG = ПНВ
.desc = Теперь ты можешь видеть в темноте! Имеет этикетку "BL CORP technology".
toggle-nightvision-verb-get-data-text = Переключить прибор ночного видения
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have it placed after/before all of the ent fields for the ease of reading.

nightvision.Enabled = !nightvision.Enabled;

if (_sharedContainer.TryGetContainingContainer(uid, out var container) &&
_inventory.TryGetSlotEntity(container.Owner, "eyes", out var entityUid) && entityUid == uid)
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
_inventory.TryGetSlotEntity(container.Owner, "eyes", out var entityUid) && entityUid == uid)
_inventory.TryGetSlotEntity(container.Owner, nightvision.Slot, out var entityUid) && entityUid == uid)

@Remuchi
Copy link
Contributor

Remuchi commented Oct 23, 2024

Also build fail not related to PR

Copy link
Contributor

@Mnemotechnician Mnemotechnician left a comment

Choose a reason for hiding this comment

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

This is just bad.

[Dependency] private readonly IOverlayManager _overlayMan = default!;
[Dependency] private readonly ILightManager _lightManager = default!;
[Dependency] private readonly IPlayerManager _playerManager = default!;
[Dependency] private readonly IEntityManager _entityManager = default!;
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
[Dependency] private readonly IEntityManager _entityManager = default!;

Comment on lines +35 to +37
var playerUid = EntityUid.Parse(_playerManager.LocalPlayer?.ControlledEntity.ToString());
var slot = _entityManager.GetComponent<InventorySlotsComponent>(playerUid);
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why in the name of all that's unholy does this convert an entity uid to a string and parse it back?!

Below is a better example. It should work, but it was made in webedit and as such may contain a mistake/mistype/whatever.

Suggested change
var playerUid = EntityUid.Parse(_playerManager.LocalPlayer?.ControlledEntity.ToString());
var slot = _entityManager.GetComponent<InventorySlotsComponent>(playerUid);
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision);
var playerUid = _playerManager.LocalPlayer?.ControlledEntity;
if (!TryComp<InventorySlotsComponent>(playerUid, out var slot) || !TryComp<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision))
return null;

Comment on lines +20 to +30
private void OnGotUnequipped(EntityUid uid, NightVisionComponent component, GotUnequippedEvent args)
{
if (args.Slot == component.Slot)
UpdateNightVisionEffects(args.Equipee, uid, false, component);
}

private void OnGotEquipped(EntityUid uid, NightVisionComponent component, GotEquippedEvent args)
{
if (args.Slot == component.Slot)
UpdateNightVisionEffects(args.Equipee, uid, true, 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 needs to account for the case where two or more clothings of the same type grant night vision (see solidus' comment about night vision helmets, etc)

Comment on lines +59 to +66
private void OnGotUnequipped(EntityUid uid, NightVisionComponent component, GotUnequippedEvent args)
{
if (args.Slot == component.Slot)
{
_overlayMan.RemoveOverlay(_overlay);
_lightManager.DrawLighting = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this needs to account for the case where two or more clothing types grant night vision at the same time.

@Peptide90
Copy link
Contributor

@Eagle-0 They were going to port their old version, which would be worse than mine. There is no toggle button and Alert. There is no color setting.

We have NVGs in Nuclear14 which is a EE fork too. Might be worth comparing, can't remember if it uses RMC code or not.

@github-actions github-actions bot added the Status: Merge Conflict FIX YOUR PR AAAGH label Jan 10, 2025
Copy link
Contributor

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

@PuroSlavKing
Copy link
Contributor

PuroSlavKing commented Jan 14, 2025

Close PR, NV already ported from WWDP

@BL02DL
Copy link
Author

BL02DL commented Jan 14, 2025

Well, yes, it's time to close this pointless pull request. He didn't stand a chance against WWDP. It was my second job, and it was definitely a bad one. I hate ss14 coding.

@BL02DL BL02DL closed this Jan 14, 2025
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: Localization Changes any ftl files Changes: Sprite Changes any png or json in an RSI Changes: YML Changes any yml files Holy Shit Priority: 3-Medium Needs to be resolved at some point Size: 3-Medium For medium issues/PRs Status: Merge Conflict FIX YOUR PR AAAGH Status: Needs Review Someone please review this Type: Feature Creation of or significant changes to a feature Type: Port Brings something to here from another codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants