Migrating fire over to the new entity system and improving the experience #2012

Sunrunner37 · 2023-03-30T04:34:52Z

Currently fires in Nitrox are entirely hit and miss. Players will douse fires in the aurora but the behavior doesn't sync correctly due to prefab spawning issues. Similarly, fires in the cyclops will spawn about 50% of the time and have no ability to persist after a server reset or reconnect. The smoke would also hang around for some players.

This PR migrates fires to the new entity system and deprecates 8 fire-specific classes.

Fires put out after reconnection:

Cooperative fire fighting:

Fires in the cyclops:

Smoke clearing after the fire:

…ence.

Jannify

WorldPersistenceTest is missing checks for CyclopsFireEntity and FireMetadata.

Jannify · 2023-05-06T15:47:03Z

NitroxClient/GameLogic/Spawning/CyclopsFireEntitySpawner.cs

+
+            if (existingFire && NitroxEntity.GetId(existingFire.gameObject) != id)
+            {
+                Log.Error($"Fire already exists at node index {nodeIndex}! Updating id to {id}");


Is this an actual error or more a warning?

Jannify · 2023-05-06T16:04:38Z

NitroxPatcher/Patches/Dynamic/Fire_Douse_Patch.cs

-                NitroxServiceLocator.LocateService<Fires>().OnDouse(__instance, amount);
-            }
+#if DEBUG
+            Log.Error($"Fire instance is unknown to the server {__instance.gameObject.name}");


Jannify · 2023-05-06T16:08:41Z

NitroxPatcher/Patches/Dynamic/Fire_Douse_Patch.cs

+            Resolve<Entities>().EntityMetadataChanged(__instance, nitroxId);
+
+            // Then tell the serve to delete the entity as it is now extinguished. 
+            Resolve<IPacketSender>().Send(new EntityDestroyed(nitroxId));


Is there a chance for race conditions and could it cause problems? Maybe we want to wait a little before deleting the entity?

I would like this packet to be the only one sent because the metadata update is an useless operation on a soon-to-be-destroyed entity. In exchange, you could add a new case in NitroxClient's EntityDestroyedProcessor for the Fire component and make it extinguished.
Therefore it'll avoid a weird case in which a metadata has 0 life in FireMetadataProcessor (which can be therefore be removed).

Jannify · 2023-05-06T16:11:30Z

NitroxPatcher/Patches/Dynamic/SubFire_CreateFire_Patch.cs

+                        NitroxId subRootId = NitroxEntity.GetId(fire.fireSubRoot.gameObject);
+                        NitroxId fireId = NitroxEntity.GetId(fire.gameObject);
+
+                        CyclopsFireEntity cyclopsFireEntity = new((int)startInRoom.roomLinks.room, nodeIndex, fireId, TechType.None.ToDto(), null, subRootId, new());


Suggested change

CyclopsFireEntity cyclopsFireEntity = new((int)startInRoom.roomLinks.room, nodeIndex, fireId, TechType.None.ToDto(), null, subRootId, new());

CyclopsFireEntity cyclopsFireEntity = new((int)startInRoom.roomLinks.room, nodeIndex, fireId, NitroxTechType.None, null, subRootId, new());

tornac1234

Needs rebase, modifications from #2063 (patches) and #1908 (replacing Nitrox.GetId) should be applied.

tornac1234 · 2023-08-05T11:02:31Z

NitroxModel/DataStructures/GameLogic/Entities/CyclopsFireEntity.cs

+
+    public override string ToString()
+    {
+        return $"[CyclopsFireEntity RoomIndex:{RoomIndex} NodeIndex:{NodeIndex} {base.ToString()}]";


Suggested change

return $"[CyclopsFireEntity RoomIndex:{RoomIndex} NodeIndex:{NodeIndex} {base.ToString()}]";

return $"[CyclopsFireEntity RoomIndex: {RoomIndex} NodeIndex: {NodeIndex} {base.ToString()}]";

tornac1234 · 2023-08-05T11:11:46Z

NitroxPatcher/Patches/Dynamic/Fire_Douse_Patch.cs

+            Resolve<ThrottledPacketSender>().RemovePendingPackets(nitroxId);
+            Resolve<Entities>().EntityMetadataChanged(__instance, nitroxId);
+
+            // Then tell the serve to delete the entity as it is now extinguished. 


Suggested change

// Then tell the serve to delete the entity as it is now extinguished.

// Then tell the server to delete the entity as it is now extinguished.

tornac1234 · 2023-08-05T11:15:38Z

NitroxPatcher/Patches/Dynamic/Fire_Douse_Patch.cs

+            Resolve<Entities>().EntityMetadataChanged(__instance, nitroxId);
+
+            // Then tell the serve to delete the entity as it is now extinguished. 
+            Resolve<IPacketSender>().Send(new EntityDestroyed(nitroxId));


I would like this packet to be the only one sent because the metadata update is an useless operation on a soon-to-be-destroyed entity. In exchange, you could add a new case in NitroxClient's EntityDestroyedProcessor for the Fire component and make it extinguished.
Therefore it'll avoid a weird case in which a metadata has 0 life in FireMetadataProcessor (which can be therefore be removed).

tornac1234 · 2023-08-05T11:25:51Z

NitroxPatcher/Patches/Dynamic/Fire_Douse_Patch.cs

+        NitroxEntity currentEntity = fire.GetComponent<NitroxEntity>();
+
+        if (currentEntity)
+        {
+            return currentEntity.Id;
+        }
+
+        NitroxEntity parentEntity = fire.transform.parent.GetComponent<NitroxEntity>();
+
+        if (parentEntity)
+        {
+            return parentEntity.Id;
+        }
+
+        return null;


After rebase, would use TryGetIdFrom to make it smaller yet understandable and also make sure the parent is still alive to avoid any possible issue.

tornac1234 · 2023-08-05T11:27:53Z

NitroxPatcher/Patches/Dynamic/SubFire_CreateFire_Patch.cs

 using NitroxModel.Helper;
+using NitroxModel.Packets;
+using NitroxModel_Subnautica.DataStructures;
 using UnityEngine;

 namespace NitroxPatcher.Patches.Dynamic


Would convert to file-scoped and rework the file entirely:

Should use Resolve<> in Prefix.

Should replace all occurences of NitroxEntity.GetId in this file by something more relevant (after rebase).

Would be way better to replace the Postfix by a Transpiler (I'm not sure if we can rely that much on this hacky solution).

tornac1234 · 2023-08-05T12:19:09Z

NitroxClient/GameLogic/Spawning/CyclopsFireEntitySpawner.cs

+        // If a fire already exists at the node, replace the old Id with the new one
+        if (fireTransform.childCount > 0)
+        {
+            Fire existingFire = fireTransform.GetComponentInChildren<Fire>();


Suggested change

Fire existingFire = fireTransform.GetComponentInChildren<Fire>();

Fire existingFire = fireTransform.GetComponentInChildren<Fire>(true);

tornac1234 · 2023-08-05T12:20:53Z

NitroxClient/GameLogic/Spawning/CyclopsFireEntitySpawner.cs

+        {
+            Fire existingFire = fireTransform.GetComponentInChildren<Fire>();
+
+            if (existingFire && NitroxEntity.GetId(existingFire.gameObject) != id)


Change GetId to something more relevant (after rebase)

tornac1234 · 2023-08-05T12:25:06Z

NitroxClient/GameLogic/Spawning/CyclopsFireEntitySpawner.cs

+
+        yield return new WaitUntil(() => newFireGameObject);
+
+        Fire fire = newFireGameObject.GetComponentInChildren<Fire>();


Suggested change

Fire fire = newFireGameObject.GetComponentInChildren<Fire>();

Fire fire = newFireGameObject.GetComponentInChildren<Fire>(true);

Migrating fire over to the new entity system and improving the experi…

d23ad6b

…ence.

Sunrunner37 requested a review from dartasen April 3, 2023 03:19

This was linked to issues Apr 9, 2023

[Lifepod] Fire extinguisher & smoke respawns on reconnect #1251

Closed

[World] Fire extinguisher only removes fire for the player using it. #1173

Open

dartasen approved these changes Apr 24, 2023

View reviewed changes

Merge branch 'master' into fires

4b223cd

dartasen requested review from Jannify and tornac1234 May 6, 2023 10:08

Jannify requested changes May 6, 2023

View reviewed changes

tornac1234 requested changes Aug 5, 2023

View reviewed changes

tornac1234 mentioned this pull request Aug 7, 2023

Sync fire extinguishers FX #1884

Open

tornac1234 added Area: persistence Related to serialization for long term storage (i.e. files and databases) Area: player Related to player character actions labels Dec 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating fire over to the new entity system and improving the experience #2012

Migrating fire over to the new entity system and improving the experience #2012

Sunrunner37 commented Mar 30, 2023

Jannify left a comment

Jannify May 6, 2023

Jannify May 6, 2023

Jannify May 6, 2023

tornac1234 Aug 5, 2023

Jannify May 6, 2023

tornac1234 left a comment

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

tornac1234 Aug 5, 2023

	CyclopsFireEntity cyclopsFireEntity = new((int)startInRoom.roomLinks.room, nodeIndex, fireId, TechType.None.ToDto(), null, subRootId, new());
	CyclopsFireEntity cyclopsFireEntity = new((int)startInRoom.roomLinks.room, nodeIndex, fireId, NitroxTechType.None, null, subRootId, new());

	return $"[CyclopsFireEntity RoomIndex:{RoomIndex} NodeIndex:{NodeIndex} {base.ToString()}]";
	return $"[CyclopsFireEntity RoomIndex: {RoomIndex} NodeIndex: {NodeIndex} {base.ToString()}]";

	// Then tell the serve to delete the entity as it is now extinguished.
	// Then tell the server to delete the entity as it is now extinguished.

	Fire existingFire = fireTransform.GetComponentInChildren<Fire>();
	Fire existingFire = fireTransform.GetComponentInChildren<Fire>(true);


		yield return new WaitUntil(() => newFireGameObject);

		Fire fire = newFireGameObject.GetComponentInChildren<Fire>();

Migrating fire over to the new entity system and improving the experience #2012

Are you sure you want to change the base?

Migrating fire over to the new entity system and improving the experience #2012

Conversation

Sunrunner37 commented Mar 30, 2023

Jannify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tornac1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment