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

1541 - Fixing lack of iron chunks in cave #2389

Conversation

Maxonovien
Copy link
Contributor

@Maxonovien Maxonovien commented Jul 6, 2021

Brief Description of What This PR Does

This PR adress the issue of lacking iron in the cave, by two fixes:

  1. Increasing small chunks density in the cave, to avoid high proportion displayed on patch map hiding sparse, very rich deposits (only affects new games as these densities are saved)
  2. Reworking spawning algorithm by running an attempt per spawner, up to max, rather than up to max attempts per spawner, to favor diversity (affects all patches).

Related Issues

Resolves #1541 and resolves #1626 and resolves #1777 .

Progress Checklist

Note: before starting this checklist the issue should be marked as non-draft.

  • PR author has checked that this PR works as intended and doesn't
    break existing features:
    https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
    (this is important as to not waste the time of Thrive team
    members reviewing this PR)
  • Initial code review passed
  • Functionality is confirmed working by another person (see above checklist link)
  • Final code review is passed and code conforms to the
    styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author.

@hhyyrylainen hhyyrylainen requested review from a team July 6, 2021 12:45
to spawn the given entity.
to spawn the given entity. Furthermore, each attempt gives a spawn chance
for each spawner (up to their type limit) to still favor diversity,
instead of one spawner taking up all the free spawning slots.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the issue that the spawners were making so much of one thing that they were reaching the maximum of living entities, which kept iron from spawning in the cave? From testing this change clearly works, but I don't understand why.

Copy link
Member

Choose a reason for hiding this comment

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

That could certainly be a thing. For a while now, I've wanted per entity spawn caps (that are more reasonable, for example I think if the 1000 entity limit fills up with cells the game will lag, a lot).

Copy link
Contributor Author

@Maxonovien Maxonovien Jul 7, 2021

Choose a reason for hiding this comment

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

That was not even the total cap, but the per-frame cap (toxyns were taking all slots (compounds would still go first as they spawned, I guess), that's why decreasing their density led to iron spawning)

Copy link
Contributor

Choose a reason for hiding this comment

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

Per-frame? As in the spawner has been adding 500 toxyns per frame? I certainly haven't observed that in game...

Copy link
Contributor Author

@Maxonovien Maxonovien Jul 7, 2021

Choose a reason for hiding this comment

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

No, the per-frame limit is yet another thing, and is much smaller (2, I think). So at each frame, 2 entities would be spawned if that did not go over total cap aswell (which makes sense, both to not have long spawning frame and to be able to spawn new things to adapt - although it could be raised shortly after new patch spawn).

Yet, given that toxins had such a high spawning rate, they always spawned up to the frame spawn limit (they roughly had ~100 chances out of 500 (max cap) so 20% chances, for 50 spawn trials or so). And thus, spawners later in the list were not tested in this frame, and the list would be reset at the next frame.

I guess that another fix could have been to save the remaining spawner list for next frame to cycle it all.

Copy link
Member

Choose a reason for hiding this comment

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

For a while now, I've wanted per entity spawn caps

Capping toxins to 100, and having a separate cap for iron and marine snow chunks, is what I meant in my comment. This doesn't have anything to do with per-frame caps, which also probably could be problematic if the toxins take up all the per-frame spawns... this is addressed by: #2138 (which is an incomplete PR to change the spawn system) which uses a round robin approach to spawn from a randomized item list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed a per-entity cap is a fully independant question, related to the total cap which is not the reason for the issues fixed here (but I agree this could prove useful overall).

This PR was really about identifying their cause and fixing them within the current system as they really harm game experience in patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also going to take a look at the incomplete spawn change PR though, see if I feel like taking it up.

Maxonovien and others added 2 commits July 7, 2021 11:36
@Maxonovien Maxonovien mentioned this pull request Jul 10, 2021
4 tasks
@@ -68,9 +68,12 @@ public class SpawnSystem
/// </summary>
private int estimateEntityCount;

private Dictionary<Spawner, int> attemptsPerSpawnType;
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be better to have the attempts counter in the Spawner class itself? Or is there somthing prohibiting this I don't see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you are right, the spawner-dependent part could be stored in Spawner indeed for memory saving.

However, given that I think I'm finally succeeding with a more efficient rework with PR #2397 (which also solves this spawn order thing), I think I'll discontinue this one for a bit, and only resume if the other PR actually does not work / takes longer and a hotfix is needed.

@FuJa0815 FuJa0815 removed the review label Jul 11, 2021
@hhyyrylainen
Copy link
Member

So this PR will be superceded by #2397 if that gets done first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
4 participants