Joachims bug fixes #8
Conversation
|
Awesome! Could you please split this into multiple PRs (bug fixes, VR support, etc.)? If these are only bug fixes, let me know. We can reopen the PR. |
| //#include "Packages/com.unity.render-pipelines.universal/ShaderLibrary/Lighting.hlsl" | ||
|
|
||
| //#include "Packages/com.unity.render-pipelines.universal/ShaderLibrary/Core.hlsl" | ||
| //#include "Packages/com.unity.render-pipelines.core/ShaderLibrary/UnityInstancing.hlsl" |
There was a problem hiding this comment.
Can this be deleted? I prefer not to have commented code unless there is a good reason for it.
This comment applies to all added lines with commented code.
| @@ -8,7 +8,7 @@ Shader "Custom/BasicVoxelShader" | |||
| SubShader | |||
There was a problem hiding this comment.
Why does the shader need to be changed? What is the bug?
| // to Opencraft-2/Packages/PolkaDOTS/Runtime/Configuration/Applicationconfig.cs | ||
| //and uncomment this line: | ||
| // return ApplicationConfig.renderDistance.Value; | ||
| //Because this breaks polkaDOTS' abstraction model, this is not desired as default |
There was a problem hiding this comment.
This is not a good solution. I would like this value to be configurable. What's the issue with the abstraction? We can also discuss on Slack if needed.
| nearbyColumns.Add(playerColumn + new int2(i,k)); | ||
| int2 targetCol = playerColumn + new int2(i, k); | ||
|
|
||
| nearbyColumns.Add(targetCol); |
There was a problem hiding this comment.
Possible debugging artifact?
| // Mark areas that need to be spawned | ||
| areasPlayersCloseTo.ExceptWith(ChunksToSpawnSnapshot); |
There was a problem hiding this comment.
Please add documentation here and throughout the code to explain what the code is doing.
In this case, this line prevents adding duplicate entries to chunksToSpawnBuffer.
Maybe we can merge this change, but it's not really the best solution imo because we are now marking columns to be generated every frame and cancel almost all of them.
Tested on a non-VR setting