b3agz / Code-A-Game-Like-Minecraft-In-Unity

Project files for a Youtube tutorial series on coding a game like Minecraft in Unity.
298 stars 237 forks source link

Previously active chunks list removal problem #9

Open smoneuse opened 4 years ago

smoneuse commented 4 years ago

https://github.com/b3agz/Code-A-Game-Like-Minecraft-In-Unity/blob/df5ace59558d3dabb46178c2a051a7697f54dde0/24-loading-and-saving/Assets/Scripts/World.cs#L283

Hello ! Thanks for this tutorial ! It's really great !!

I think there is a problem in this part of the code. As you said in one of your video, when you remove an element from the list, the indexes are recalculated. In your checkViewDistance method, you use an index to find the chunk that needs to be kept. that's ok, but why does the for loop goes on (with the indexes recalculated) once the chunk is found ?

In my opinion, it you be better if there was this code : (with a break once the chunk found)

                    for (int i=0; i< previouslyActiveChunks.Count; i++) {
                        if (previouslyActiveChunks[i].Equals(chunks[x, z].coord)) {
                            previouslyActiveChunks.RemoveAt(i);
                            break;                            
                        }
                    }

Or even better, as the method Equals exists for the ChunkCoord class :
previouslyActiveChunks.Remove(chunks[x, z].coord);

Thanks again for your videos !! A real pleasure to watch !! Cheers from France !

smoneuse commented 4 years ago

For some reason I don't figure yet, some ChunksCoords are present multiple times in previouslyActiveChunks. I think this is due to activeChunks Add() calls in various places in the code, with new Instances of ChunkCoord...

However, maybe the better way for this issue is to browse the list from end to beginning :

                for (int i = previouslyActiveChunks.Count - 1; i >= 0; i--) {
                    if (previouslyActiveChunks[i].Equals(new ChunkCoord(x, z))) { 
                        previouslyActiveChunks.RemoveAt(i);
                    }
                }