Case'd into a corner

- Do not clear instance handles when clearing an instancer
  - Fixes case where a stolen handle gets cleared after changing
    instancers
- Use a concurrent queue for initializing instancers in DrawManager
  - Fixes race condition where only one of two instancers created in the
    same moment get initialized
This commit is contained in:
Jozufozu 2024-05-01 19:30:11 -07:00
parent dbf1977c0d
commit f51e2d2a1b
2 changed files with 15 additions and 3 deletions

View file

@ -161,7 +161,16 @@ public abstract class AbstractInstancer<I extends Instance> implements Instancer
* Clear all instances without freeing resources.
*/
public void clear() {
handles.forEach(InstanceHandleImpl::clear);
for (InstanceHandleImpl handle : handles) {
// Only clear instances that belong to this instancer.
// If one of these handles was stolen by another instancer,
// clearing it here would cause significant visual artifacts and instance leaks.
// At the same time, we need to clear handles we own to prevent
// instances from changing/deleting positions in this instancer that no longer exist.
if (handle.instancer == this) {
handle.clear();
}
}
instances.clear();
handles.clear();
changed.clear();

View file

@ -4,7 +4,9 @@ import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import com.jozufozu.flywheel.Flywheel;
import com.jozufozu.flywheel.api.backend.Engine;
@ -32,7 +34,7 @@ public abstract class DrawManager<N extends AbstractInstancer<?>> {
* <br>
* All new instancers land here before having resources allocated in {@link #flush}.
*/
protected final List<UninitializedInstancer<N, ?>> initializationQueue = new ArrayList<>();
protected final Queue<UninitializedInstancer<N, ?>> initializationQueue = new ConcurrentLinkedQueue<>();
@SuppressWarnings("unchecked")
public <I extends Instance> Instancer<I> getInstancer(Environment environment, InstanceType<I> type, Model model, RenderStage stage) {
@ -72,7 +74,8 @@ public abstract class DrawManager<N extends AbstractInstancer<?>> {
// Only queue the instancer for initialization if it has anything to render.
if (checkAndWarnEmptyModel(key.model())) {
// Thread safety: this method is called atomically from within computeIfAbsent,
// so we don't need extra synchronization to protect the queue.
// so you'd think we don't need extra synchronization to protect the queue, but
// somehow threads can race here and wind up never initializing an instancer.
initializationQueue.add(new UninitializedInstancer<>(key, out));
}
return out;