-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
1560 regular spawn #2397
1560 regular spawn #2397
Conversation
…rive into 1560_regular_spawn
Co-authored-by: Jan <[email protected]>
…rive into 1560_regular_spawn
FillAndShuffle(); | ||
} | ||
|
||
return currentContent[currentContent.Count - 1]; |
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 are you picking the last element? Picking the first one saves one calculation and count-call. This is even more important if you use a LinkedList
.
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 would guess that it's because the list is popped from the back (as popping from the front is very bad for performance).
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.
Yes, this was precisely done to pop from the back indeed. Worth mentionning in a comment though.
src/general/ShuffleBag.cs
Outdated
private List<T> initialContent; | ||
private List<T> currentContent; | ||
|
||
public ShuffleBag(Random random) |
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 think more reusability is always good here. An optional boolean that defines if the bag should be refilled automatically would be great. When set to false this class would behave like a randomized Stack or Queue, which could have it's future usages.
public ShuffleBag(Random random) | |
public ShuffleBag(Random random, bool refillWhenEmpty = true) |
public void Add(T element) | ||
{ | ||
initialContent.Add(element); | ||
currentContent.Add(element); |
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.
Shouldn't the new element be inserted at a random index? (.Insert(random.Next(...))
)
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.
Hm, that could be a way to go, but you can still shuffle the bag afterwards if you really need to. I see it as putting something in your irl bag: unless you shake it, that is the first thing you'll be able to pick afterwards.
I'm not sure which behavior is the best one. The current one only adds further possibilities, but perhaps we don't want them to be a thing.
If kept as such, I think I should specify this non-random behavior in a comment nonetheless.
src/microbe_stage/SpawnSystem.cs
Outdated
private int estimateEntityCountInSpawnRadius; | ||
|
||
/// <summary> | ||
/// Last recorded position of the player. Positions are recorded upon leaving the immobility zone. | ||
/// </summary> | ||
private Vector3 lastRecordedPlayerPosition; |
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 prefer inline initialization here to match the rest of the class.
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 agree with you but I have a faint memory of having a reason to avoid it. But this does not seem necessary from looking at the code, so maybe I'm mistaken. I'll take a look.
Co-authored-by: Jan <[email protected]>
…rive into 1560_regular_spawn
fixed the enumerator not actually following the enumerator interface
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'm happy with the changes that are here, but this does not actually solve #1560 so that remains an issue, and for the future there's a problem as each spawner is not fully tapped before potentially moving onto the next one.
I'm fine with merging this if you are fine with my changes.
probably was addressed, dismissing to get this in for the next release
Here's a list I made to keep track of what to specifically test and what I found on each of them.
Let me know if I missed anything. |
That's a very comprehensive list. Sorry I was in a hurry and merged this before you were done. I'll link that as a good example of very thorough testing on our wiki on the testing page. https://wiki.revolutionarygamesstudio.com/wiki/Testing |
That's a great testing review, thanks! If I get it well you raise caveats for 2. and 3. For 3., you could indeed call it a "belt spawning" case, but you can't really avoid it because we make things spawn around the player (approximately on the border ), as we do not want things to close nor far away from the player. The precedent issue was that belt spawning would make contiguous spawning from an edge of the screen to the other, so my way to fix it was to just limit the amount of entity. Such patterns are thus fully part of the algorithm, which I don't think is much of an issue, or at least not one the PR intended to fix. For 2., it is not a surprise that iron is over-represented. But are other compounds not spawned enough? In other words, do you encounter significantly less of them when compared to a patch with similar values for other compounds? Normally, the spawns should be independant for each compound over the long run (you may have some more local interactions, though). |
So yeah the other compounds do spawn but very rarely it seems. It took me probably around 5 minutes before I finally found a patch of phosphate. All I was seeing for 5 minutes was only iron clouds, iron particles, and ice shards. Eventually, I did find more patches of compounds and it was a bit more frequent after I found the phosphate cloud but when I saved and loaded it back it was back to rarely finding any other compounds (but maybe this time it didn't take as long). |
Brief Description of What This PR Does
This PR takes on #2138 (algorithms only) to rework the spawn system. It tries to stick to the old implementation.
It brings two major changes:
Note: further issues are related but could be dealt with another PR:
Related Issues
Fixes #1550 . Closes #1739
Also resolves #1541 and resolves #1626 and resolves #1777 . Partly overlaps with and closes #2389.
closes #2138 (previous PR)
Progress Checklist
Note: before starting this checklist the issue should be marked as non-draft.
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)
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.