Skip to content

fix oxygen tank is empty in inventory when reconnecting #2302

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Buivol16
Copy link

@Buivol16 Buivol16 commented Mar 9, 2025

Fix for issue: #2266

@Measurity Measurity added this to the 1.8 milestone Mar 10, 2025
@Measurity Measurity added the Area: items Related to items and inventories label Mar 10, 2025
Comment on lines +51 to +53
var iOxygenSource = pickupable.gameObject.GetComponent<IOxygenSource>();
var entityMetadataUpdate = GetEntityMetadataUpdateForIOxygenMetadata(iOxygenSource, itemId);
if (entityMetadataUpdate != null) packetSender.Send(entityMetadataUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a very specific check when we have methods to extract metadata in a general fashion. Did you test any weird behaviour when sending every extractable metadata?
@tornac1234 what is your opinion on this?

@@ -47,6 +48,9 @@ public void BroadcastItemAdd(Pickupable pickupable, Transform containerTransform

if (packetSender.Send(new EntityReparented(itemId, ownerId)))
{
var iOxygenSource = pickupable.gameObject.GetComponent<IOxygenSource>();
Copy link
Member

Choose a reason for hiding this comment

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

We don't use var at all.
It seems you don't get visual feedback from your IDE for this. We have a .editorconfig file in the project which should setup out code style for you... 🤔

@@ -47,6 +48,9 @@ public void BroadcastItemAdd(Pickupable pickupable, Transform containerTransform

if (packetSender.Send(new EntityReparented(itemId, ownerId)))
{
var iOxygenSource = pickupable.gameObject.GetComponent<IOxygenSource>();
var entityMetadataUpdate = GetEntityMetadataUpdateForIOxygenMetadata(iOxygenSource, itemId);
if (entityMetadataUpdate != null) packetSender.Send(entityMetadataUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

Always use brackets for ifs.

@@ -39,6 +39,7 @@ namespace NitroxModel.DataStructures.GameLogic.Entities.Metadata
[ProtoInclude(79, typeof(SeaTreaderMetadata))]
[ProtoInclude(80, typeof(StayAtLeashPositionMetadata))]
[ProtoInclude(81, typeof(EggMetadata))]
[ProtoInclude(82, typeof(IOxygenSourceMetadata))]
Copy link
Member

Choose a reason for hiding this comment

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

Should add tests for it in WorldPersistenceTest

Comment on lines +11 to +13
IOxygenSource iOxygenSource = gameObject.GetComponent<IOxygenSource>();

if (iOxygenSource != null)
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
IOxygenSource iOxygenSource = gameObject.GetComponent<IOxygenSource>();
if (iOxygenSource != null)
if (gameObject.TryGetComponent(out IOxygenSource iOxygenSource))

Makes it a little bit clearer imo. Also you should never compare unity objects against null (see here for explaination), instead threat it like in c++ as a bool


if (iOxygenSource != null)
{
iOxygenSource.AddOxygen(metadata.AvailableOxygen);
Copy link
Member

Choose a reason for hiding this comment

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

As this can also possibly be executed on existing tanks I would first calculate the delta of GetOxygenAvailable() and metadata.AvailableOxygen, to then use it for AddOxygen() or RemoveOxygen() depending on plus or minus.

@@ -26,7 +27,7 @@ public ItemContainers(IPacketSender packetSender, EntityMetadataManager entityMe
this.entityMetadataManager = entityMetadataManager;
}

public void BroadcastItemAdd(Pickupable pickupable, Transform containerTransform)
public void BroadcastItemAddAndSendMetadata(Pickupable pickupable, Transform containerTransform)
Copy link
Member

Choose a reason for hiding this comment

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

Also with this changes the value only gets updated when adding the tank to the inventory and not when we use breath the air in it or rise to the surface. Maybe we need some kind of system to send this data repentantly (like every 1-3 seconds or so).

@dartasen dartasen modified the milestones: 1.8, 1.9 Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: items Related to items and inventories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants