Cloud spawns happen on regular intervals, and conform to the compound densities of a patch. (closes 1560) #2138
Conversation
Gwen-Fay
changed the title
There was a problem hiding this comment.
Overall this makes the spawn system much more complicated as there's now in fact two separate systems in there: the clouds and other stuff. I'd prefer to go like all the way at once (also make cells and chunks spawn using a timer with weighted distribution), or retrofit the new cloud logic as much as possible to the existing system.
src/microbe_stage/MicrobeStage.cs
Outdated
| @@ -252,7 +252,7 @@ public void SetupStage() | |||
| if (IsLoadedFromSave) | |||
| { | |||
| HUD.OnEnterStageTransition(false); | |||
| UpdatePatchSettings(true); | |||
| UpdatePatchSettings(true, Player.Transform.origin); | |||
There was a problem hiding this comment.
Why did you change this to require the player position? The player position is already passed in on the spawn cycle, it shouldn't require setup here.
There was a problem hiding this comment.
When testing my changes, it felt weird when you change patches and the screen is empty. So I decided to quickly added some free cloud spawns on patch change. In hindsight, yeah this isn't a good way to implement that.
There was a problem hiding this comment.
I think it is a good change to make some stuff spawn on patch change. I think in the cloud setup, there's already a parameter or something that detects when the patch changes (as it should clear the old spawners). At least the code that calls the patch setup should know if a patch change is happening, and could pass that information to either here or the spawn system, which might be a bit nicer way to do it (basically reset the spawn intervals to cause something to spawn immediately).
src/microbe_stage/PatchManager.cs
Outdated
| { | ||
| GD.Print("Number of clouds in this patch = ", biome.Compounds.Count); | ||
|
|
||
| foreach (var entry in biome.Compounds) | ||
| spawnSystem.ClearBiomeCompounds(); |
There was a problem hiding this comment.
This shouldn't always happen here, this should only be triggered when the spawners are destroyed on patch change.
There was a problem hiding this comment.
I've heard the eventual goal is to have compound amounts and densities change over time. This would mean clearing the system here. I guess you are right though, that's currently not a feature, so it makes no sense now.
src/microbe_stage/Spawners.cs
Outdated
| @@ -342,24 +341,27 @@ public override IEnumerable<ISpawned> Spawn(Node worldNode, Vector3 location) | |||
| /// <summary> | |||
| /// Spawns compound clouds of a certain type | |||
| /// </summary> | |||
| public class CompoundCloudSpawner : Spawner | |||
| public class CloudSpawner : Spawner | |||
There was a problem hiding this comment.
Couldn't the design work so that the CompoundCloudSpawner class is left as is, with the SpawnSystem handling the selecting which clouds are allowed to spawn?
There was a problem hiding this comment.
The old system had multipleCompoundCloudSpawners, one per compound. The new system has only one CloudSpawner, where the compounds are selected with the cloudBag. I figured this justified the name change.
There was a problem hiding this comment.
I did gather that from the changes. However, I think in order to decouple the spawn setup, from the actual spawning logic, you'll need to reintroduce back this intermediate CompoundCloudSpawner per type. Because as I said the spawn setup should not care at all what the player position is. The spawn system can have internally used a CloudSpawner that keeps track of extra state that is constructed from the registered CompoundCloudSpawner instances.
I'm being pretty hard on these changes, because the spawn system is a huge problem, and making it extra complicated with special logic for one type of spawning, might mean that it will be even more difficult to get someone to work on the spawn system again. Even with the current logic, which I think is pretty minimal for the random spawning it currently does, it's been years that I've tried to nudge people into working on the system. That's why I'd like to see a unified spawner design to avoid unnecessary complexity that will make future changes harder.
Though this issue mentions only clouds: #1560 I think people would welcome having all the spawners moved over to a time based spawn algorithm that would still take the different densities into account.
There was a problem hiding this comment.
I understand why you are being hard on this, something like a spawn system should be easy to understand and change. I will rework it when I have free time.
There was a problem hiding this comment.
I look forward to that. It would be great to finally get the spawn system into better shape.
While pondering this a bit more, I think it could in fact simplify the design if the spawners are owned by the spawn system (and stored in saves, though getting delegates to save properly is not entirely trivial), and then the patch conditions could read the existing data, and communicate through method calls the necessary updates (as the spawn system probably needs to react to the changes, so it isn't a good idea to let the patch conditions applier to directly modify the spawner properties). But anyway, if you don't want to dig into the saving part yet, you can keep the current intermediate spawner objects, which as I said, make the behaviour regarding saving easier to not break.
|
I think it wouldn't be too hard to just switch over every Spawner to this system. I could have a classes of Spawns, and the bag system can just pop and handle the Spawn. This would remove the Spawners themselves and simplify alot. |
But you still would need some kind of tracking for what things want to spawn, right? I think quite a bit of the SpawnSystem state is not saved, instead the patch settings are re-applied when a save is loaded. That could be changed, but then the patch settings object would be much more tightly tied to the spawn system as it needs to look at its internal state to determine which spawns to add, remove and adjust. |
|
I have tested this PR in-game and am pleased with how some compounds immediately spawn upon migration. However; Compound spawn rates seem to have diminished in occurrence overall making all patches equally devoid of sufficient compound clouds. It would also be nice if the system included chunk spawning as well, but maybe you just didn't get to that yet. I feel like this method could work assuming that the interval is easily tunable. But it is difficult to fully gauge how much more effective it is than our current system with the current scarcity of resulting resources. |
|
Ok I think I fixed all of the issues you have pointed out. As for the QueuedSpawn, right now I just added it into the current system. This means a spawn event (or in rare cases 2) fills up a Queue with SpawnItems, and when spawning if the SpawnItem has multiple spawns, an IEnumerator gets created and looped through over several frames. |
|
I think that was basically about how the current spawn system does the queued spawns over multiple frames. |
|
Yeah, I basically just copied the existing system. I don't like how there is essentially 2 queue like things, but I can't think of a way to make it so each SpawnItem can only spawn one thing without adding more complexity and rewriting colony spawns. Oh well, it works and it's not that complicated. |
I have re-requested HH on this. |
|
Unless something was done about my concerns of leaking objects, that's a definite problem that needs addressing somehow (I suggested that design choice moving back to the enumerator style spawners). Edit: and my other concern (in cause I didn't mention this) is that creating all of the colony members on a single frame is definitely more expensive than just creating an enumerator and reading items one by one, so that only one scene is instantiated at once. |
I thought they did, at least thats the jist I got from their comment. Sorry for bugging you hh, in that case nevertmind. |
At least github doesn't show that any new commits were added after that comment 18 days ago, so I assume it wasn't addressed by any commit... Edit: Or was that addressed by an earlier commit? From a quick look now, I still see parts of the design where the spawn system is too closely coupled to what it is trying to spawn (for example it looks like it has an explicit method to add microbe spawner in it, and the different spawn types don't seem to implement a single interface, so the spawn system is too tightly coupled to the things it spawns) |
|
I'm not sure if it's better to limit the spawning density. See what a mess I've got around me after leaving it untouched for a minute! (I'm sorry I commented in the wrong PR)
|
|
Unless you specifically tested on this PR's code, then I don't think that problem is relevant to report here. Because this entirely changes the just chance based all things spawn randomly approach to a round robin timed bag of stuff to spawn approach. |
I assume this is a simple problem being able to be solved here quick. This is a common problem. I've tested on branch master and this PR. (What about another issue?) |
|
Do you mean that, that screenshot is the situation on this PR's version? If so, then I guess this PR would be good if this addressed the too much stuff on screen at once problem. |
|
This screenshot is taken on branch master. But similar situation can happen on this PR. Here's the screenshot: |
|
The author of this PR said that they don't have time to finish this. So this PR is now looking for another person to finish it. |
|
A new PR is going to replace this: #2397 |
This replaces the old system for one that's like a bag. Compounds are added to the bag and shuffled. When the spawn er needs a cloud, it draws one from the bag. When the bag is empty, the bag is refilled and shuffled. This ensures that you can get a steady rate of each compound, but the order of spawn is still random.
The compound bag is set up so that it will empty the bag at a predefined constant rate that can be changed for game-play balance.
closes #1560