-
Notifications
You must be signed in to change notification settings - Fork 215
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
Conversation
RSI Diff Bot; head commit 6161b11 merging into 9430b90 Resources/Textures/Clothing/Eyes/NVG/diagnosticnightvision.rsi
Resources/Textures/Clothing/Eyes/NVG/mednightvision.rsi
Resources/Textures/Clothing/Eyes/NVG/nightvision.rsi
Resources/Textures/Clothing/Eyes/NVG/secnightvision.rsi
|
I would wait and port NVG's from WD instead, they have thermals too |
@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. |
There was a problem hiding this 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!
Resources/Textures/_LostParadise/Clothing/Eyes/NVG/diagnosticnightvision.rsi/meta.json
Outdated
Show resolved
Hide resolved
Resources/Textures/_LostParadise/Clothing/Eyes/NVG/mednightvision.rsi/meta.json
Outdated
Show resolved
Hide resolved
Resources/Textures/_LostParadise/Clothing/Eyes/NVG/nightvision.rsi/meta.json
Outdated
Show resolved
Hide resolved
Resources/Textures/_LostParadise/Clothing/Eyes/NVG/secnightvision.rsi/meta.json
Outdated
Show resolved
Hide resolved
Can you also provide a "Media", how the nightvsion looks like when on? at least the default. |
There was a problem hiding this 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.
@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.. |
There was a problem hiding this 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.
{ | ||
var playerUid = EntityUid.Parse(_playerManager.LocalPlayer?.ControlledEntity.ToString()); | ||
var slot = _entityManager.GetComponent<InventorySlotsComponent>(playerUid); | ||
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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
There was a problem hiding this 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.
- type: entity | ||
id: ActionBaseToggleNightVision | ||
name: Toggle NVG | ||
description: Toggles the NVG on and off. | ||
noSpawn: true | ||
components: | ||
- type: InstantAction | ||
itemIconStyle: NoItem | ||
event: !type:ToggleNightVisionEvent | ||
|
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 = Переключить прибор ночного видения |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_inventory.TryGetSlotEntity(container.Owner, "eyes", out var entityUid) && entityUid == uid) | |
_inventory.TryGetSlotEntity(container.Owner, nightvision.Slot, out var entityUid) && entityUid == uid) |
Also build fail not related to PR |
There was a problem hiding this 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!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Dependency] private readonly IEntityManager _entityManager = default!; |
var playerUid = EntityUid.Parse(_playerManager.LocalPlayer?.ControlledEntity.ToString()); | ||
var slot = _entityManager.GetComponent<InventorySlotsComponent>(playerUid); | ||
_entityManager.TryGetComponent<NightVisionComponent>(slot.SlotData["eyes"].HeldEntity, out var nightvision); |
There was a problem hiding this comment.
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.
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; |
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); | ||
} |
There was a problem hiding this comment.
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)
private void OnGotUnequipped(EntityUid uid, NightVisionComponent component, GotUnequippedEvent args) | ||
{ | ||
if (args.Slot == component.Slot) | ||
{ | ||
_overlayMan.RemoveOverlay(_overlay); | ||
_lightManager.DrawLighting = true; | ||
} | ||
} |
There was a problem hiding this comment.
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.
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. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Close PR, NV already ported from WWDP |
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. |
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