From a7e709086662ffdec21be7a4e7859b4aa0a33692 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Mon, 28 Oct 2024 21:53:38 -0700 Subject: [PATCH 01/18] Finding an old bookmark - Make AbstractInstancer much more slim and move logic to BaseInstancer - Extend paging concept to the indirect instancer - Extend ObjectStorage to support more interesting layouts - Instance creation on indirect is now entirely lock free and deletions no longer require re-uploading the entire instancer --- .../backend/engine/AbstractInstancer.java | 229 +-------------- .../backend/engine/BaseInstancer.java | 175 +++++++++++ .../backend/engine/InstanceHandleImpl.java | 3 +- .../engine/indirect/IndirectInstancer.java | 273 ++++++++++++++++-- .../engine/indirect/ObjectStorage.java | 27 +- .../engine/instancing/InstancedInstancer.java | 58 +++- .../flywheel/internal/indirect/cull.glsl | 10 +- 7 files changed, 511 insertions(+), 264 deletions(-) create mode 100644 common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java index 3215d12d2..95bc88e58 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java @@ -1,27 +1,14 @@ package dev.engine_room.flywheel.backend.engine; -import java.util.ArrayList; - -import org.jetbrains.annotations.Nullable; - import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceType; import dev.engine_room.flywheel.api.instance.Instancer; import dev.engine_room.flywheel.backend.engine.embed.Environment; -import dev.engine_room.flywheel.backend.util.AtomicBitSet; -public abstract class AbstractInstancer implements Instancer, InstanceHandleImpl.State { +public abstract class AbstractInstancer implements Instancer { public final InstanceType type; public final Environment environment; - private final Recreate recreate; - - // Lock for all instances, only needs to be used in methods that may run on the TaskExecutor. - protected final Object lock = new Object(); - protected final ArrayList instances = new ArrayList<>(); - protected final ArrayList> handles = new ArrayList<>(); - - protected final AtomicBitSet changed = new AtomicBitSet(); - protected final AtomicBitSet deleted = new AtomicBitSet(); + public final Recreate recreate; protected AbstractInstancer(InstancerKey key, Recreate recreate) { this.type = key.type(); @@ -29,218 +16,16 @@ protected AbstractInstancer(InstancerKey key, Recreate recreate) { this.recreate = recreate; } - @Override - public InstanceHandleImpl.State setChanged(int index) { - notifyDirty(index); - return this; - } - - @Override - public InstanceHandleImpl.State setDeleted(int index) { - notifyRemoval(index); - return InstanceHandleImpl.Deleted.instance(); - } - - @Override - public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { - if (visible) { - return this; - } - - notifyRemoval(index); - - I instance; - synchronized (lock) { - // I think we need to lock to prevent wacky stuff from happening if the array gets resized. - instance = instances.get(index); - } - - return new InstanceHandleImpl.Hidden<>(recreate, instance); - } - - @Override - public I createInstance() { - var handle = new InstanceHandleImpl<>(this); - I instance = type.create(handle); - - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - return instance; - } - } - - public void revealInstance(InstanceHandleImpl handle, I instance) { - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - } - } - - @Override - public void stealInstance(@Nullable I instance) { - if (instance == null) { - return; - } - - var instanceHandle = instance.handle(); - - if (!(instanceHandle instanceof InstanceHandleImpl)) { - // UB: do nothing - return; - } - - // Should InstanceType have an isInstance method? - @SuppressWarnings("unchecked") - var handle = (InstanceHandleImpl) instanceHandle; - - // No need to steal if this instance is already owned by this instancer. - if (handle.state == this) { - return; - } - // Not allowed to steal deleted instances. - if (handle.state instanceof InstanceHandleImpl.Deleted) { - return; - } - // No need to steal if the instance will recreate to us. - if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { - return; - } - - // FIXME: in theory there could be a race condition here if the instance - // is somehow being stolen by 2 different instancers between threads. - // That seems kinda impossible so I'm fine leaving it as is for now. - - // Add the instance to this instancer. - if (handle.state instanceof AbstractInstancer other) { - // Remove the instance from its old instancer. - // This won't have any unwanted effect when the old instancer - // is filtering deleted instances later, so is safe. - other.notifyRemoval(handle.index); - - handle.state = this; - // Only lock now that we'll be mutating our state. - synchronized (lock) { - handle.index = instances.size(); - addLocked(instance, handle); - } - } else if (handle.state instanceof InstanceHandleImpl.Hidden) { - handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); - } - } - - /** - * Calls must be synchronized on {@link #lock}. - */ - private void addLocked(I instance, InstanceHandleImpl handle) { - instances.add(instance); - handles.add(handle); - setIndexChanged(handle.index); - } - - public int instanceCount() { - return instances.size(); - } + public abstract InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance); - public void notifyDirty(int index) { - if (index < 0 || index >= instanceCount()) { - return; - } - setIndexChanged(index); - } + public abstract int instanceCount(); - protected void setIndexChanged(int index) { - changed.set(index); - } - - public void notifyRemoval(int index) { - if (index < 0 || index >= instanceCount()) { - return; - } - deleted.set(index); - } - - public void removeDeletedInstances() { - if (deleted.isEmpty()) { - return; - } - - // Figure out which elements are to be removed. - final int oldSize = this.instances.size(); - int removeCount = deleted.cardinality(); - - if (oldSize == removeCount) { - clear(); - return; - } - - final int newSize = oldSize - removeCount; - - // Start from the first deleted index. - int writePos = deleted.nextSetBit(0); - - if (writePos < newSize) { - // Since we'll be shifting everything into this space we can consider it all changed. - setRangeChanged(writePos, newSize); - } - - // We definitely shouldn't consider the deleted instances as changed though, - // else we might try some out of bounds accesses later. - changed.clear(newSize, oldSize); - - // Punch out the deleted instances, shifting over surviving instances to fill their place. - for (int scanPos = writePos; (scanPos < oldSize) && (writePos < newSize); scanPos++, writePos++) { - // Find next non-deleted element. - scanPos = deleted.nextClearBit(scanPos); - - if (scanPos != writePos) { - // Grab the old instance/handle from scanPos... - var handle = handles.get(scanPos); - I instance = instances.get(scanPos); - - // ... and move it to writePos. - handles.set(writePos, handle); - instances.set(writePos, instance); - - // Make sure the handle knows it's been moved - handle.index = writePos; - } - } - - deleted.clear(); - instances.subList(newSize, oldSize) - .clear(); - handles.subList(newSize, oldSize) - .clear(); - } - - protected void setRangeChanged(int start, int end) { - changed.set(start, end); - } - - /** - * Clear all instances without freeing resources. - */ - public void 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.state == this) { - handle.clear(); - handle.state = InstanceHandleImpl.Deleted.instance(); - } - } - instances.clear(); - handles.clear(); - changed.clear(); - deleted.clear(); - } + public abstract void removeDeletedInstances(); public abstract void delete(); + public abstract void clear(); + @Override public String toString() { return "AbstractInstancer[" + instanceCount() + ']'; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java new file mode 100644 index 000000000..eae6d73cb --- /dev/null +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/BaseInstancer.java @@ -0,0 +1,175 @@ +package dev.engine_room.flywheel.backend.engine; + +import java.util.ArrayList; + +import org.jetbrains.annotations.Nullable; + +import dev.engine_room.flywheel.api.instance.Instance; +import dev.engine_room.flywheel.backend.util.AtomicBitSet; + +public abstract class BaseInstancer extends AbstractInstancer implements InstanceHandleImpl.State { + // Lock for all instances, only needs to be used in methods that may run on the TaskExecutor. + protected final Object lock = new Object(); + protected final ArrayList instances = new ArrayList<>(); + protected final ArrayList> handles = new ArrayList<>(); + + protected final AtomicBitSet changed = new AtomicBitSet(); + protected final AtomicBitSet deleted = new AtomicBitSet(); + + protected BaseInstancer(InstancerKey key, Recreate recreate) { + super(key, recreate); + } + + + @Override + public InstanceHandleImpl.State setChanged(int index) { + notifyDirty(index); + return this; + } + + @Override + public InstanceHandleImpl.State setDeleted(int index) { + notifyRemoval(index); + return InstanceHandleImpl.Deleted.instance(); + } + + @Override + public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { + if (visible) { + return this; + } + + notifyRemoval(index); + + I instance; + synchronized (lock) { + // I think we need to lock to prevent wacky stuff from happening if the array gets resized. + instance = instances.get(index); + } + + return new InstanceHandleImpl.Hidden<>(recreate, instance); + } + + @Override + public I createInstance() { + var handle = new InstanceHandleImpl<>(this); + I instance = type.create(handle); + + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + return instance; + } + } + + public InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance) { + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + } + return this; + } + + @Override + public void stealInstance(@Nullable I instance) { + if (instance == null) { + return; + } + + var instanceHandle = instance.handle(); + + if (!(instanceHandle instanceof InstanceHandleImpl)) { + // UB: do nothing + return; + } + + // Should InstanceType have an isInstance method? + @SuppressWarnings("unchecked") var handle = (InstanceHandleImpl) instanceHandle; + + // No need to steal if this instance is already owned by this instancer. + if (handle.state == this) { + return; + } + // Not allowed to steal deleted instances. + if (handle.state instanceof InstanceHandleImpl.Deleted) { + return; + } + // No need to steal if the instance will recreate to us. + if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { + return; + } + + // FIXME: in theory there could be a race condition here if the instance + // is somehow being stolen by 2 different instancers between threads. + // That seems kinda impossible so I'm fine leaving it as is for now. + + // Add the instance to this instancer. + if (handle.state instanceof BaseInstancer other) { + // Remove the instance from its old instancer. + // This won't have any unwanted effect when the old instancer + // is filtering deleted instances later, so is safe. + other.notifyRemoval(handle.index); + + handle.state = this; + // Only lock now that we'll be mutating our state. + synchronized (lock) { + handle.index = instances.size(); + addLocked(instance, handle); + } + } else if (handle.state instanceof InstanceHandleImpl.Hidden) { + handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); + } + } + + /** + * Calls must be synchronized on {@link #lock}. + */ + private void addLocked(I instance, InstanceHandleImpl handle) { + instances.add(instance); + handles.add(handle); + setIndexChanged(handle.index); + } + + public int instanceCount() { + return instances.size(); + } + + public void notifyDirty(int index) { + if (index < 0 || index >= instanceCount()) { + return; + } + setIndexChanged(index); + } + + protected void setIndexChanged(int index) { + changed.set(index); + } + + public void notifyRemoval(int index) { + if (index < 0 || index >= instanceCount()) { + return; + } + deleted.set(index); + } + + /** + * Clear all instances without freeing resources. + */ + public void 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.state == this) { + handle.clear(); + handle.state = InstanceHandleImpl.Deleted.instance(); + } + } + instances.clear(); + handles.clear(); + changed.clear(); + deleted.clear(); + } +} diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java index d5932f45e..445f5dfba 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java @@ -62,8 +62,7 @@ public State setVisible(InstanceHandleImpl handle, int index, boolean visi return this; } var instancer = recreate.recreate(); - instancer.revealInstance(handle, instance); - return instancer; + return instancer.revealInstance(handle, instance); } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index ee11999a8..c09a3e9e9 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -1,8 +1,12 @@ package dev.engine_room.flywheel.backend.engine.indirect; +import java.lang.reflect.Array; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; import org.lwjgl.system.MemoryUtil; @@ -10,17 +14,22 @@ import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceWriter; import dev.engine_room.flywheel.backend.engine.AbstractInstancer; +import dev.engine_room.flywheel.backend.engine.InstanceHandleImpl; import dev.engine_room.flywheel.backend.engine.InstancerKey; import dev.engine_room.flywheel.backend.util.AtomicBitSet; import dev.engine_room.flywheel.lib.math.MoreMath; public class IndirectInstancer extends AbstractInstancer { + private final AtomicReference pages; + private final long instanceStride; private final InstanceWriter writer; private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; private final AtomicBitSet changedPages = new AtomicBitSet(); + private final AtomicBitSet fullPages = new AtomicBitSet(); + private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -33,18 +42,93 @@ public IndirectInstancer(InstancerKey key, Recreate recreate) { .byteSize()); writer = this.type.writer(); boundingSphere = key.model().boundingSphere(); - } - @Override - public void setIndexChanged(int index) { - changedPages.set(ObjectStorage.objectIndex2PageIndex(index)); + instanceClass = (Class) type.create(new InstanceHandleImpl(null)) + .getClass(); + pages = new AtomicReference<>((InstancePage[]) Array.newInstance(InstancePage.class, 0)); } - @Override - protected void setRangeChanged(int start, int end) { - super.setRangeChanged(start, end); + public final class InstancePage implements InstanceHandleImpl.State { + private final int pageNo; + private final I[] instances; + private final InstanceHandleImpl[] handles; + /** + * A bitset describing which indices in the instances/handles arrays contain live instances. + */ + private final AtomicInteger valid; + + InstancePage(Class clazz, int pageNo) { + this.pageNo = pageNo; + this.instances = (I[]) Array.newInstance(clazz, ObjectStorage.PAGE_SIZE); + this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; + this.valid = new AtomicInteger(0); + } + + public boolean add(I instance, InstanceHandleImpl handle) { + // Thread safety: we loop until we either win the race and add the given instance, or we + // run out of space because other threads trying to add at the same time. + while (true) { + int currentValue = valid.get(); + if (currentValue == 0xFFFFFFFF) { + // The page is full, must search elsewhere + return false; + } + + // determine what the new long value will be after we set the appropriate bit. + int index = Integer.numberOfTrailingZeros(~currentValue); + + int newValue = currentValue | (1 << index); + + // if no other thread has modified the value since we read it, we won the race and we are done. + if (valid.compareAndSet(currentValue, newValue)) { + instances[index] = instance; + handles[index] = handle; + handle.state = this; + handle.index = (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + + changedPages.set(pageNo); + if (newValue == 0xFFFFFFFF) { + fullPages.set(pageNo); + } + return true; + } + } + } + + @Override + public InstanceHandleImpl.State setChanged(int index) { + changedPages.set(pageNo); + return this; + } + + @Override + public InstanceHandleImpl.State setDeleted(int index) { + int localIndex = index % ObjectStorage.PAGE_SIZE; - changedPages.set(ObjectStorage.objectIndex2PageIndex(start), ObjectStorage.objectIndex2PageIndex(end) + 1); + instances[localIndex] = null; + handles[localIndex] = null; + + while (true) { + int currentValue = valid.get(); + int newValue = currentValue & ~(1 << localIndex); + + if (valid.compareAndSet(currentValue, newValue)) { + fullPages.clear(pageNo); + return InstanceHandleImpl.Deleted.instance(); + } + } + } + + @Override + public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { + if (visible) { + return this; + } + + int localIndex = index % ObjectStorage.PAGE_SIZE; + + return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); + } } public void addDraw(IndirectDraw draw) { @@ -56,9 +140,18 @@ public List draws() { } public void update(int modelIndex, int baseInstance) { - this.modelIndex = modelIndex; this.baseInstance = baseInstance; - mapping.update(modelIndex, instanceCount()); + + if (this.modelIndex == modelIndex && changedPages.isEmpty()) { + return; + } + this.modelIndex = modelIndex; + var pages = this.pages.get(); + mapping.updateCount(pages.length); + + for (int i = 0; i < pages.length; i++) { + mapping.updatePage(i, modelIndex, pages[i].valid.get()); + } } public void writeModel(long ptr) { @@ -76,21 +169,12 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { return; } - int numPages = mapping.pageCount(); - - var instanceCount = instances.size(); - - for (int page = changedPages.nextSetBit(0); page >= 0 && page < numPages; page = changedPages.nextSetBit(page + 1)) { - int startObject = ObjectStorage.pageIndex2ObjectIndex(page); - - if (startObject >= instanceCount) { - break; - } - - int endObject = Math.min(instanceCount, ObjectStorage.pageIndex2ObjectIndex(page + 1)); + var pages = this.pages.get(); + for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + var instances = pages[page].instances; long baseByte = mapping.page2ByteOffset(page); - long size = (endObject - startObject) * instanceStride; + long size = ObjectStorage.PAGE_SIZE * instanceStride; // Because writes are broken into pages, we end up with significantly more calls into // StagingBuffer#enqueueCopy and the allocations for the writer got out of hand. Here @@ -101,9 +185,10 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { long direct = stagingBuffer.reserveForCopy(size, instanceVbo, baseByte); if (direct != MemoryUtil.NULL) { - for (int i = startObject; i < endObject; i++) { - var instance = instances.get(i); - writer.write(direct, instance); + for (I instance : instances) { + if (instance != null) { + writer.write(direct, instance); + } direct += instanceStride; } continue; @@ -112,9 +197,10 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { // Otherwise, write to a scratch buffer and enqueue a copy. var block = stagingBuffer.getScratch(size); var ptr = block.ptr(); - for (int i = startObject; i < endObject; i++) { - var instance = instances.get(i); - writer.write(ptr, instance); + for (I instance : instances) { + if (instance != null) { + writer.write(ptr, instance); + } ptr += instanceStride; } stagingBuffer.enqueueCopy(block.ptr(), size, instanceVbo, baseByte); @@ -123,6 +209,10 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { changedPages.clear(); } + public void removeDeletedInstances() { + + } + @Override public void delete() { for (IndirectDraw draw : draws()) { @@ -143,4 +233,129 @@ public int baseInstance() { public int local2GlobalInstanceIndex(int instanceIndex) { return mapping.objectIndex2GlobalIndex(instanceIndex); } + + @Override + public I createInstance() { + var handle = new InstanceHandleImpl(null); + I instance = type.create(handle); + + addInner(instance, handle); + + return instance; + } + + public InstanceHandleImpl.State revealInstance(InstanceHandleImpl handle, I instance) { + addInner(instance, handle); + return handle.state; + } + + @Override + public void stealInstance(@Nullable I instance) { + if (instance == null) { + return; + } + + var instanceHandle = instance.handle(); + + if (!(instanceHandle instanceof InstanceHandleImpl)) { + // UB: do nothing + return; + } + + // Should InstanceType have an isInstance method? + @SuppressWarnings("unchecked") var handle = (InstanceHandleImpl) instanceHandle; + + // Not allowed to steal deleted instances. + if (handle.state instanceof InstanceHandleImpl.Deleted) { + return; + } + // No need to steal if the instance will recreate to us. + if (handle.state instanceof InstanceHandleImpl.Hidden hidden && recreate.equals(hidden.recreate())) { + return; + } + + // FIXME: in theory there could be a race condition here if the instance + // is somehow being stolen by 2 different instancers between threads. + // That seems kinda impossible so I'm fine leaving it as is for now. + + // Add the instance to this instancer. + if (handle.state instanceof InstancePage other) { + // TODO: shortcut here if we already own the instance + + // Remove the instance from its old instancer. + // This won't have any unwanted effect when the old instancer + // is filtering deleted instances later, so is safe. + other.setDeleted(handle.index); + + // Only lock now that we'll be mutating our state. + addInner(instance, handle); + } else if (handle.state instanceof InstanceHandleImpl.Hidden) { + handle.state = new InstanceHandleImpl.Hidden<>(recreate, instance); + } + } + + private void addInner(I instance, InstanceHandleImpl handle) { + // Outer loop: + // - try to find an empty space + // - or grow the page array if we can't + // - add the instance to the new page, or try again + while (true) { + var pages = this.pages.get(); + + // First, try to find a page with space. + for (int i = fullPages.nextClearBit(0); i < pages.length; i = fullPages.nextClearBit(i + 1)) { + // It may have been filled in while we were searching, but hopefully not. + if (pages[i].add(instance, handle)) { + return; + } + } + + // If we're here, all other pages are full + // If we hit this on the second iteration of the outer loop then `pages` is once again full. + var desiredLength = pages.length + 1; + + // Inner loop: grow the page array. This is very similar to the logic in AtomicBitSet. + while (pages.length < desiredLength) { + // Thread safety: segments contains all pages from the currently visible pages, plus extra. + // all pages in the currently visible pages are canonical and will not change. + // Can't just `new InstancePage[]` because it has a generic parameter. + InstancePage[] newPages = (InstancePage[]) Array.newInstance(InstancePage.class, desiredLength); + + System.arraycopy(pages, 0, newPages, 0, pages.length); + newPages[pages.length] = new InstancePage(instanceClass, pages.length); + + // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. + if (this.pages.compareAndSet(pages, newPages)) { + pages = newPages; + } else { + // If we "lose the race" and are growing the AtomicBitset segments larger, + // then we will gather the new canonical pages from the update which we missed on the next iteration of this loop. + // The new page will be discarded and never seen again. + pages = this.pages.get(); + } + } + + // Shortcut: try to add the instance to the last page. + // Technically we could just let the outer loop go again, but that + // involves a good bit of work just to likely get back here. + if (pages[pages.length - 1].add(instance, handle)) { + return; + } + // It may be the case that many other instances were added in the same instant. + // We can still lose this race, though it is very unlikely. + } + } + + public int instanceCount() { + // Not exactly accurate but it's an upper bound. + // TODO: maybe this could be tracked with an AtomicInteger? + return pages.get().length << ObjectStorage.LOG_2_PAGE_SIZE; + } + + /** + * Clear all instances without freeing resources. + */ + public void clear() { + + } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 56d39613e..bf12c46a1 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -14,6 +14,7 @@ public class ObjectStorage extends AbstractArena { public static final int PAGE_MASK = PAGE_SIZE - 1; public static final int INITIAL_PAGES_ALLOCATED = 4; + public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; /** * The GPU side buffer containing all the objects, logically divided into page frames. @@ -37,8 +38,8 @@ public ObjectStorage(long objectSizeBytes) { this.frameDescriptorBuffer = new ResizableStorageBuffer(); objectBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * elementSizeBytes); - frameDescriptorBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * Integer.BYTES); - frameDescriptors = MemoryBlock.malloc(INITIAL_PAGES_ALLOCATED * Integer.BYTES); + frameDescriptorBuffer.ensureCapacity(INITIAL_PAGES_ALLOCATED * DESCRIPTOR_SIZE_BYTES); + frameDescriptors = MemoryBlock.malloc(INITIAL_PAGES_ALLOCATED * DESCRIPTOR_SIZE_BYTES); } public Mapping createMapping() { @@ -79,7 +80,7 @@ public void delete() { } private long ptrForPage(int page) { - return frameDescriptors.ptr() + (long) page * Integer.BYTES; + return frameDescriptors.ptr() + (long) page * DESCRIPTOR_SIZE_BYTES; } public static int objectIndex2PageIndex(int objectIndex) { @@ -100,6 +101,14 @@ public class Mapping { private int modelIndex = -1; private int objectCount = 0; + public void updatePage(int i, int modelIndex, int i1) { + var ptr = ptrForPage(pages[i]); + MemoryUtil.memPutInt(ptr, modelIndex); + MemoryUtil.memPutInt(ptr + 4, i1); + + ObjectStorage.this.needsUpload = true; + } + /** * Adjust this allocation to the given model index and object count. * @@ -155,6 +164,18 @@ public void update(int modelIndex, int objectCount) { } } + public void updateCount(int newLength) { + var oldLength = pages.length; + if (oldLength > newLength) { + // Eagerly free the now unnecessary pages. + // shrink will zero out the pageTable entries for the freed pages. + shrink(oldLength, newLength); + } else if (oldLength < newLength) { + // Allocate new pages to fit the new object count. + grow(newLength, oldLength); + } + } + public int pageCount() { return pages.length; } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java index 2895195a5..833e90ddf 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java @@ -7,7 +7,7 @@ import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceWriter; -import dev.engine_room.flywheel.backend.engine.AbstractInstancer; +import dev.engine_room.flywheel.backend.engine.BaseInstancer; import dev.engine_room.flywheel.backend.engine.InstancerKey; import dev.engine_room.flywheel.backend.gl.TextureBuffer; import dev.engine_room.flywheel.backend.gl.buffer.GlBuffer; @@ -15,7 +15,7 @@ import dev.engine_room.flywheel.lib.math.MoreMath; import dev.engine_room.flywheel.lib.memory.MemoryBlock; -public class InstancedInstancer extends AbstractInstancer { +public class InstancedInstancer extends BaseInstancer { private final int instanceStride; private final InstanceWriter writer; @@ -109,6 +109,60 @@ public boolean needsToGrow(long capacity) { return capacity > vbo.size(); } + public void removeDeletedInstances() { + if (deleted.isEmpty()) { + return; + } + + // Figure out which elements are to be removed. + final int oldSize = this.instances.size(); + int removeCount = deleted.cardinality(); + + if (oldSize == removeCount) { + clear(); + return; + } + + final int newSize = oldSize - removeCount; + + // Start from the first deleted index. + int writePos = deleted.nextSetBit(0); + + if (writePos < newSize) { + // Since we'll be shifting everything into this space we can consider it all changed. + changed.set(writePos, newSize); + } + + // We definitely shouldn't consider the deleted instances as changed though, + // else we might try some out of bounds accesses later. + changed.clear(newSize, oldSize); + + // Punch out the deleted instances, shifting over surviving instances to fill their place. + for (int scanPos = writePos; (scanPos < oldSize) && (writePos < newSize); scanPos++, writePos++) { + // Find next non-deleted element. + scanPos = deleted.nextClearBit(scanPos); + + if (scanPos != writePos) { + // Grab the old instance/handle from scanPos... + var handle = handles.get(scanPos); + I instance = instances.get(scanPos); + + // ... and move it to writePos. + handles.set(writePos, handle); + instances.set(writePos, instance); + + // Make sure the handle knows it's been moved + handle.index = writePos; + } + } + + deleted.clear(); + instances.subList(newSize, oldSize) + .clear(); + handles.subList(newSize, oldSize) + .clear(); + } + public void delete() { if (vbo == null) { return; diff --git a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl index 395979d5c..1869e33b8 100644 --- a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl +++ b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl @@ -126,24 +126,22 @@ bool _flw_isVisible(uint instanceIndex, uint modelIndex) { } void main() { - uint pageIndex = gl_WorkGroupID.x; + uint pageIndex = gl_WorkGroupID.x << 1u; if (pageIndex >= _flw_pageFrameDescriptors.length()) { return; } - uint packedModelIndexAndCount = _flw_pageFrameDescriptors[pageIndex]; + uint modelIndex = _flw_pageFrameDescriptors[pageIndex]; - uint pageInstanceCount = packedModelIndexAndCount >> _FLW_PAGE_COUNT_OFFSET; + uint pageValidity = _flw_pageFrameDescriptors[pageIndex + 1]; - if (gl_LocalInvocationID.x >= pageInstanceCount) { + if (((1u << gl_LocalInvocationID.x) & pageValidity) == 0) { return; } uint instanceIndex = gl_GlobalInvocationID.x; - uint modelIndex = packedModelIndexAndCount & _FLW_MODEL_INDEX_MASK; - if (_flw_isVisible(instanceIndex, modelIndex)) { uint localIndex = atomicAdd(_flw_models[modelIndex].instanceCount, 1); uint targetIndex = _flw_models[modelIndex].baseInstance + localIndex; From 20b3f78b9cadbf5e3c0eb455b0dcb142c81ce1fe Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Tue, 29 Oct 2024 20:01:20 -0700 Subject: [PATCH 02/18] A real page turner - Try to shuffle over instances into pages with space - Clear out now-unused logic from ObjectStorage - Some cleanup and more comments in IndirectInstancer --- .../backend/engine/AbstractInstancer.java | 2 +- .../flywheel/backend/engine/DrawManager.java | 2 +- .../engine/indirect/IndirectInstancer.java | 143 ++++++++++++++++-- .../engine/indirect/ObjectStorage.java | 85 ----------- .../engine/instancing/InstancedInstancer.java | 2 +- 5 files changed, 135 insertions(+), 99 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java index 95bc88e58..605ee0441 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/AbstractInstancer.java @@ -20,7 +20,7 @@ protected AbstractInstancer(InstancerKey key, Recreate recreate) { public abstract int instanceCount(); - public abstract void removeDeletedInstances(); + public abstract void parallelUpdate(); public abstract void delete(); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java index f34887f00..898253bb0 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java @@ -50,7 +50,7 @@ public AbstractInstancer getInstancer(InstancerKey ke public Plan createFramePlan() { // Go wide on instancers to process deletions in parallel. - return ForEachPlan.of(() -> new ArrayList<>(instancers.values()), AbstractInstancer::removeDeletedInstances); + return ForEachPlan.of(() -> new ArrayList<>(instancers.values()), AbstractInstancer::parallelUpdate); } public void flush(LightStorage lightStorage, EnvironmentStorage environmentStorage) { diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index c09a3e9e9..3bec5e4f7 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -6,6 +6,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; @@ -20,13 +21,12 @@ import dev.engine_room.flywheel.lib.math.MoreMath; public class IndirectInstancer extends AbstractInstancer { - private final AtomicReference pages; - private final long instanceStride; private final InstanceWriter writer; private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; + private final AtomicReference pages; private final AtomicBitSet changedPages = new AtomicBitSet(); private final AtomicBitSet fullPages = new AtomicBitSet(); private final Class instanceClass; @@ -45,7 +45,19 @@ public IndirectInstancer(InstancerKey key, Recreate recreate) { instanceClass = (Class) type.create(new InstanceHandleImpl(null)) .getClass(); - pages = new AtomicReference<>((InstancePage[]) Array.newInstance(InstancePage.class, 0)); + pages = new AtomicReference<>(pageArray(0)); + } + + @NotNull + @SuppressWarnings("unchecked") + private InstancePage[] pageArray(int length) { + return (InstancePage[]) Array.newInstance(InstancePage.class, length); + } + + @NotNull + @SuppressWarnings("unchecked") + private I[] instanceArray() { + return (I[]) Array.newInstance(instanceClass, ObjectStorage.PAGE_SIZE); } public final class InstancePage implements InstanceHandleImpl.State { @@ -57,19 +69,30 @@ public final class InstancePage implements InstanceHandleImpl.State { */ private final AtomicInteger valid; - InstancePage(Class clazz, int pageNo) { + InstancePage(int pageNo) { this.pageNo = pageNo; - this.instances = (I[]) Array.newInstance(clazz, ObjectStorage.PAGE_SIZE); + this.instances = instanceArray(); this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; this.valid = new AtomicInteger(0); } + public int count() { + return Integer.bitCount(valid.get()); + } + + /** + * Attempt to add the given instance/handle to this page. + * + * @param instance The instance to add + * @param handle The instance's handle + * @return true if the instance was added, false if the page is full + */ public boolean add(I instance, InstanceHandleImpl handle) { // Thread safety: we loop until we either win the race and add the given instance, or we // run out of space because other threads trying to add at the same time. while (true) { int currentValue = valid.get(); - if (currentValue == 0xFFFFFFFF) { + if (isFull(currentValue)) { // The page is full, must search elsewhere return false; } @@ -84,10 +107,13 @@ public boolean add(I instance, InstanceHandleImpl handle) { instances[index] = instance; handles[index] = handle; handle.state = this; - handle.index = (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + // Handle index is unique amongst all pages of this instancer. + handle.index = local2HandleIndex(index); changedPages.set(pageNo); - if (newValue == 0xFFFFFFFF) { + if (isFull(newValue)) { + // The page is now full, mark it so in the bitset. + // This is safe because only one bit position changes at a time. fullPages.set(pageNo); } return true; @@ -95,6 +121,10 @@ public boolean add(I instance, InstanceHandleImpl handle) { } } + private int local2HandleIndex(int index) { + return (pageNo << ObjectStorage.LOG_2_PAGE_SIZE) + index; + } + @Override public InstanceHandleImpl.State setChanged(int index) { changedPages.set(pageNo); @@ -114,6 +144,7 @@ public InstanceHandleImpl.State setDeleted(int index) { if (valid.compareAndSet(currentValue, newValue)) { fullPages.clear(pageNo); + changedPages.set(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -129,6 +160,56 @@ public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } + + public int takeFrom(InstancePage other) { + // Fill the holes in this page with instances from the other page. + + int valid = this.valid.get(); + int otherValid = other.valid.get(); + + // If the other page is empty, or we're full, we're done. + if (otherValid == 0 || valid == 0xFFFFFFFF) { + return valid; + } + + // Something is going to change, so mark stuff ahead of time. + changedPages.set(pageNo); + changedPages.set(other.pageNo); + + for (int i = 0; i < ObjectStorage.PAGE_SIZE; i++) { + int mask = 1 << i; + + if ((otherValid & mask) == 0) { + continue; + } + + int writePos = Integer.numberOfTrailingZeros(~valid); + + instances[writePos] = other.instances[i]; + handles[writePos] = other.handles[i]; + + handles[writePos].state = this; + handles[writePos].index = local2HandleIndex(writePos); + + // Clear out the other page. + otherValid &= ~mask; + other.handles[i] = null; + other.instances[i] = null; + + // Set the bit in this page and find the next write position. + valid |= 1 << writePos; + + // If we're full, we're done. + if (isFull(valid)) { + break; + } + } + + this.valid.set(valid); + other.valid.set(otherValid); + + return valid; + } } public void addDraw(IndirectDraw draw) { @@ -209,8 +290,45 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { changedPages.clear(); } - public void removeDeletedInstances() { + public void parallelUpdate() { + if (true) { + // FIXME: infinite loop when the page in readpos doesn't have enough to fill the page in writepos + return; + } + + var pages = this.pages.get(); + + // If there are at least 2 pages with space, we can consolidate. + if (fullPages.cardinality() > (pages.length - 2)) { + return; + } + // Note this runs after visuals are updated so we don't really have to take care for thread safety. + + int writePos = 0; + + while (true) { + writePos = fullPages.nextClearBit(writePos); + int readPos = fullPages.nextClearBit(writePos + 1); + + if (writePos >= pages.length || readPos >= pages.length) { + break; + } + + InstancePage writeTo = pages[writePos]; + InstancePage readFrom = pages[readPos]; + + int validNow = writeTo.takeFrom(readFrom); + + if (isFull(validNow)) { + fullPages.set(writePos); + writePos = readPos; + } + } + } + + private static boolean isFull(int valid) { + return valid == 0xFFFFFFFF; } @Override @@ -319,10 +437,10 @@ private void addInner(I instance, InstanceHandleImpl handle) { // Thread safety: segments contains all pages from the currently visible pages, plus extra. // all pages in the currently visible pages are canonical and will not change. // Can't just `new InstancePage[]` because it has a generic parameter. - InstancePage[] newPages = (InstancePage[]) Array.newInstance(InstancePage.class, desiredLength); + InstancePage[] newPages = pageArray(desiredLength); System.arraycopy(pages, 0, newPages, 0, pages.length); - newPages[pages.length] = new InstancePage(instanceClass, pages.length); + newPages[pages.length] = new InstancePage(pages.length); // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. if (this.pages.compareAndSet(pages, newPages)) { @@ -356,6 +474,9 @@ public int instanceCount() { * Clear all instances without freeing resources. */ public void clear() { + this.pages.set(pageArray(0)); + changedPages.clear(); + fullPages.clear(); } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index bf12c46a1..3f43b8374 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -98,9 +98,6 @@ public class Mapping { private static final int[] EMPTY_ALLOCATION = new int[0]; private int[] pages = EMPTY_ALLOCATION; - private int modelIndex = -1; - private int objectCount = 0; - public void updatePage(int i, int modelIndex, int i1) { var ptr = ptrForPage(pages[i]); MemoryUtil.memPutInt(ptr, modelIndex); @@ -109,61 +106,6 @@ public void updatePage(int i, int modelIndex, int i1) { ObjectStorage.this.needsUpload = true; } - /** - * Adjust this allocation to the given model index and object count. - * - *

This method triggers eager resizing of the allocation to fit the new object count. - * If the model index is different from the current one, all frame descriptors will be updated. - * - * @param modelIndex The model index the objects in this allocation are associated with. - * @param objectCount The number of objects in this allocation. - */ - public void update(int modelIndex, int objectCount) { - boolean incremental = this.modelIndex == modelIndex; - - if (incremental && objectCount == this.objectCount) { - // Nothing will change. - return; - } - - ObjectStorage.this.needsUpload = true; - - this.modelIndex = modelIndex; - this.objectCount = objectCount; - - var oldLength = pages.length; - var newLength = objectIndex2PageIndex((objectCount + PAGE_MASK)); - - if (oldLength > newLength) { - // Eagerly free the now unnecessary pages. - // shrink will zero out the pageTable entries for the freed pages. - shrink(oldLength, newLength); - - if (incremental) { - // Only update the last page, everything else is unchanged. - updateRange(newLength - 1, newLength); - } - } else if (oldLength < newLength) { - // Allocate new pages to fit the new object count. - grow(newLength, oldLength); - - if (incremental) { - // Update the old last page + all new pages - updateRange(oldLength - 1, newLength); - } - } else { - if (incremental) { - // Only update the last page. - updateRange(oldLength - 1, oldLength); - } - } - - if (!incremental) { - // Update all pages. - updateRange(0, newLength); - } - } - public void updateCount(int newLength) { var oldLength = pages.length; if (oldLength > newLength) { @@ -189,37 +131,10 @@ public void delete() { ObjectStorage.this.free(page); } pages = EMPTY_ALLOCATION; - modelIndex = -1; - objectCount = 0; ObjectStorage.this.needsUpload = true; } - /** - * Calculates the page descriptor for the given page index. - * Runs under the assumption than all pages are full except maybe the last one. - */ - private int calculatePageDescriptor(int pageIndex) { - int countInPage; - if (objectCount % PAGE_SIZE != 0 && pageIndex == pages.length - 1) { - // Last page && it isn't full -> use the remainder. - countInPage = objectCount & PAGE_MASK; - } else if (objectCount > 0) { - // Full page. - countInPage = PAGE_SIZE; - } else { - // Empty page, this shouldn't be reachable because we eagerly free empty pages. - countInPage = 0; - } - return (modelIndex & 0x3FFFFF) | (countInPage << 26); - } - - private void updateRange(int start, int oldLength) { - for (int i = start; i < oldLength; i++) { - MemoryUtil.memPutInt(ptrForPage(pages[i]), calculatePageDescriptor(i)); - } - } - private void grow(int neededPages, int oldLength) { pages = Arrays.copyOf(pages, neededPages); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java index 833e90ddf..c56b281eb 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedInstancer.java @@ -109,7 +109,7 @@ public boolean needsToGrow(long capacity) { return capacity > vbo.size(); } - public void removeDeletedInstances() { + public void parallelUpdate() { if (deleted.isEmpty()) { return; } From fac63168c1c445883867f48dafc54b987a84bba8 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 1 Nov 2024 12:47:15 -0700 Subject: [PATCH 03/18] Bookkeeping - Mappings drop pages when they write zero validity bits - Instancer only updates pages that changed --- .../engine/indirect/IndirectInstancer.java | 59 +++++++------------ .../engine/indirect/ObjectStorage.java | 54 +++++++++++++++-- 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index 3bec5e4f7..8ee084136 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -168,7 +168,7 @@ public int takeFrom(InstancePage other) { int otherValid = other.valid.get(); // If the other page is empty, or we're full, we're done. - if (otherValid == 0 || valid == 0xFFFFFFFF) { + if (isEmpty(otherValid) || isFull(valid)) { return valid; } @@ -223,15 +223,27 @@ public List draws() { public void update(int modelIndex, int baseInstance) { this.baseInstance = baseInstance; - if (this.modelIndex == modelIndex && changedPages.isEmpty()) { + var sameModelIndex = this.modelIndex == modelIndex; + if (sameModelIndex && changedPages.isEmpty()) { + // Nothing to do! return; } + this.modelIndex = modelIndex; + var pages = this.pages.get(); mapping.updateCount(pages.length); - for (int i = 0; i < pages.length; i++) { - mapping.updatePage(i, modelIndex, pages[i].valid.get()); + if (sameModelIndex) { + // Only need to update the changed pages. + for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + mapping.updatePage(page, modelIndex, pages[page].valid.get()); + } + } else { + // Need to update all pages since the model index changed. + for (int i = 0; i < pages.length; i++) { + mapping.updatePage(i, modelIndex, pages[i].valid.get()); + } } } @@ -291,46 +303,17 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { } public void parallelUpdate() { - if (true) { - // FIXME: infinite loop when the page in readpos doesn't have enough to fill the page in writepos - return; - } - - var pages = this.pages.get(); - - // If there are at least 2 pages with space, we can consolidate. - if (fullPages.cardinality() > (pages.length - 2)) { - return; - } - - // Note this runs after visuals are updated so we don't really have to take care for thread safety. - - int writePos = 0; - - while (true) { - writePos = fullPages.nextClearBit(writePos); - int readPos = fullPages.nextClearBit(writePos + 1); - - if (writePos >= pages.length || readPos >= pages.length) { - break; - } - - InstancePage writeTo = pages[writePos]; - InstancePage readFrom = pages[readPos]; - - int validNow = writeTo.takeFrom(readFrom); - - if (isFull(validNow)) { - fullPages.set(writePos); - writePos = readPos; - } - } + // TODO: Merge pages when they're less than half full. } private static boolean isFull(int valid) { return valid == 0xFFFFFFFF; } + private static boolean isEmpty(int otherValid) { + return otherValid == 0; + } + @Override public void delete() { for (IndirectDraw draw : draws()) { diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 3f43b8374..20b6ee87a 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -13,6 +13,8 @@ public class ObjectStorage extends AbstractArena { public static final int PAGE_SIZE = 1 << LOG_2_PAGE_SIZE; public static final int PAGE_MASK = PAGE_SIZE - 1; + public static final int INVALID_PAGE = -1; + public static final int INITIAL_PAGES_ALLOCATED = 4; public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; @@ -53,8 +55,13 @@ public long byteCapacity() { @Override public void free(int i) { + if (i == INVALID_PAGE) { + return; + } super.free(i); - MemoryUtil.memPutInt(ptrForPage(i), 0); + var ptr = ptrForPage(i); + MemoryUtil.memPutInt(ptr, 0); + MemoryUtil.memPutInt(ptr + 4, 0); } @Override @@ -98,14 +105,49 @@ public class Mapping { private static final int[] EMPTY_ALLOCATION = new int[0]; private int[] pages = EMPTY_ALLOCATION; - public void updatePage(int i, int modelIndex, int i1) { - var ptr = ptrForPage(pages[i]); + public void updatePage(int index, int modelIndex, int validBits) { + if (validBits == 0) { + holePunch(index); + return; + } + var page = pages[index]; + + if (page == INVALID_PAGE) { + // Un-holed punch. + page = unHolePunch(index); + } + + var ptr = ptrForPage(page); MemoryUtil.memPutInt(ptr, modelIndex); - MemoryUtil.memPutInt(ptr + 4, i1); + MemoryUtil.memPutInt(ptr + 4, validBits); ObjectStorage.this.needsUpload = true; } + /** + * Free a page on the inside of the mapping, maintaining the same virtual mapping size. + * + * @param index The index of the page to free. + */ + public void holePunch(int index) { + ObjectStorage.this.free(pages[index]); + pages[index] = INVALID_PAGE; + + ObjectStorage.this.needsUpload = true; + } + + /** + * Allocate a new page on the inside of the mapping, maintaining the same virtual mapping size. + * + * @param index The index of the page to allocate. + * @return The allocated page. + */ + private int unHolePunch(int index) { + int page = ObjectStorage.this.alloc(); + pages[index] = page; + return page; + } + public void updateCount(int newLength) { var oldLength = pages.length; if (oldLength > newLength) { @@ -122,8 +164,8 @@ public int pageCount() { return pages.length; } - public long page2ByteOffset(int page) { - return ObjectStorage.this.byteOffsetOf(pages[page]); + public long page2ByteOffset(int index) { + return ObjectStorage.this.byteOffsetOf(pages[index]); } public void delete() { From fc3e475ec9e096bd4f9dc6bf4f5815782f42a97d Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 1 Nov 2024 23:26:12 -0700 Subject: [PATCH 04/18] Gattai! - Combine pages only when they're at most half full, and not empty - This guarantees that we'll fully empty a page, allowing us to free the memory for use by other instancers - Track mergeable pages via a separate bitset --- .../engine/indirect/IndirectInstancer.java | 65 +++++++++++++++---- .../flywheel/backend/util/AtomicBitSet.java | 8 +++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index 8ee084136..b3a032337 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -29,6 +29,12 @@ public class IndirectInstancer extends AbstractInstancer private final AtomicReference pages; private final AtomicBitSet changedPages = new AtomicBitSet(); private final AtomicBitSet fullPages = new AtomicBitSet(); + /** + * The set of mergable pages. A page is mergeable if it is not empty and has 16 or fewer instances. + * These constraints are set so that we can guarantee that merging two pages leaves one entirely empty, + * but we also don't want to waste work merging into pages that are already empty. + */ + private final AtomicBitSet mergeablePages = new AtomicBitSet(); private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -116,6 +122,13 @@ public boolean add(I instance, InstanceHandleImpl handle) { // This is safe because only one bit position changes at a time. fullPages.set(pageNo); } + if (isEmpty(currentValue)) { + // Value we just saw was zero, so since we added something we are now mergeable! + mergeablePages.set(pageNo); + } else if (Integer.bitCount(currentValue) == 16) { + // We just filled the 17th instance, so we are no longer mergeable. + mergeablePages.clear(pageNo); + } return true; } } @@ -143,8 +156,16 @@ public InstanceHandleImpl.State setDeleted(int index) { int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - fullPages.clear(pageNo); changedPages.set(pageNo); + if (isEmpty(newValue)) { + // If we decremented to zero then we're no longer mergeable. + mergeablePages.clear(pageNo); + } else if (Integer.bitCount(newValue) == 16) { + // If we decremented to 16 then we're now mergeable. + mergeablePages.set(pageNo); + } + // Set full page last so that other threads don't race to set the other bitsets. + fullPages.clear(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -161,17 +182,12 @@ public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } - public int takeFrom(InstancePage other) { + public void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); int otherValid = other.valid.get(); - // If the other page is empty, or we're full, we're done. - if (isEmpty(otherValid) || isFull(valid)) { - return valid; - } - // Something is going to change, so mark stuff ahead of time. changedPages.set(pageNo); changedPages.set(other.pageNo); @@ -208,7 +224,13 @@ public int takeFrom(InstancePage other) { this.valid.set(valid); other.valid.set(otherValid); - return valid; + mergeablePages.set(pageNo, isMergeable(valid)); + // With all assumptions in place we should have fully drained the other page, but check just to be safe. + mergeablePages.set(other.pageNo, isMergeable(otherValid)); + + if (isFull(valid)) { + fullPages.set(pageNo); + } } } @@ -303,15 +325,36 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { } public void parallelUpdate() { - // TODO: Merge pages when they're less than half full. + var pages = this.pages.get(); + + int page = 0; + while (mergeablePages.cardinality() > 1) { + page = mergeablePages.nextSetBit(page); + if (page < 0) { + break; + } + + // Find the next mergeable page. + int next = mergeablePages.nextSetBit(page + 1); + if (next < 0) { + break; + } + + // Try to merge the pages. + pages[page].takeFrom(pages[next]); + } } private static boolean isFull(int valid) { return valid == 0xFFFFFFFF; } - private static boolean isEmpty(int otherValid) { - return otherValid == 0; + private static boolean isEmpty(int valid) { + return valid == 0; + } + + private static boolean isMergeable(int valid) { + return !isEmpty(valid) && Integer.bitCount(valid) <= 16; } @Override diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java b/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java index 334b02732..beeeb403d 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/util/AtomicBitSet.java @@ -43,6 +43,14 @@ public AtomicBitSet(int log2SegmentSizeInBits, int numBitsToPreallocate) { segments = new AtomicReference<>(new AtomicBitSetSegments(numSegmentsToPreallocate, numLongsPerSegment)); } + public void set(int position, boolean value) { + if (value) { + set(position); + } else { + clear(position); + } + } + public void set(int position) { int longPosition = longIndexInSegmentForPosition(position); From a9f2018c0a267216bdc30cf8f30e258d23a7b2be Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sat, 2 Nov 2024 14:01:22 -0700 Subject: [PATCH 05/18] The epitome of laziness - Only upload changed page frame descriptors - In the instancer, track changed contents separately from changed validity, so we can separately upload objects and descriptors --- .../engine/indirect/IndirectInstancer.java | 69 ++++++++++++++----- .../engine/indirect/ObjectStorage.java | 43 +++++++----- 2 files changed, 76 insertions(+), 36 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index b3a032337..d6a73d61e 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -27,7 +27,19 @@ public class IndirectInstancer extends AbstractInstancer private final Vector4fc boundingSphere; private final AtomicReference pages; - private final AtomicBitSet changedPages = new AtomicBitSet(); + /** + * The set of pages whose count changed and thus need their descriptor re-uploaded. + */ + private final AtomicBitSet validityChanged = new AtomicBitSet(); + /** + * The set of pages whose content changed and thus need their instances re-uploaded. + * Note that we don't re-upload for deletions, as the memory becomes invalid and masked out by the validity bits. + */ + private final AtomicBitSet contentsChanged = new AtomicBitSet(); + /** + * The set of pages that are entirely full. + * We scan the clear bits of this set when trying to add an instance. + */ private final AtomicBitSet fullPages = new AtomicBitSet(); /** * The set of mergable pages. A page is mergeable if it is not empty and has 16 or fewer instances. @@ -69,6 +81,7 @@ private I[] instanceArray() { public final class InstancePage implements InstanceHandleImpl.State { private final int pageNo; private final I[] instances; + // Handles are only read in #takeFrom. It would be nice to avoid tracking these at all. private final InstanceHandleImpl[] handles; /** * A bitset describing which indices in the instances/handles arrays contain live instances. @@ -116,7 +129,8 @@ public boolean add(I instance, InstanceHandleImpl handle) { // Handle index is unique amongst all pages of this instancer. handle.index = local2HandleIndex(index); - changedPages.set(pageNo); + contentsChanged.set(pageNo); + validityChanged.set(pageNo); if (isFull(newValue)) { // The page is now full, mark it so in the bitset. // This is safe because only one bit position changes at a time. @@ -140,7 +154,7 @@ private int local2HandleIndex(int index) { @Override public InstanceHandleImpl.State setChanged(int index) { - changedPages.set(pageNo); + contentsChanged.set(pageNo); return this; } @@ -156,7 +170,7 @@ public InstanceHandleImpl.State setDeleted(int index) { int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - changedPages.set(pageNo); + validityChanged.set(pageNo); if (isEmpty(newValue)) { // If we decremented to zero then we're no longer mergeable. mergeablePages.clear(pageNo); @@ -182,19 +196,21 @@ public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); } - public void takeFrom(InstancePage other) { + /** + * Only call this on 2 pages that are mergeable. + * + * @param other The page to take instances from. + */ + private void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); int otherValid = other.valid.get(); - // Something is going to change, so mark stuff ahead of time. - changedPages.set(pageNo); - changedPages.set(other.pageNo); - for (int i = 0; i < ObjectStorage.PAGE_SIZE; i++) { int mask = 1 << i; + // Find set bits in the other page. if ((otherValid & mask) == 0) { continue; } @@ -212,7 +228,7 @@ public void takeFrom(InstancePage other) { other.handles[i] = null; other.instances[i] = null; - // Set the bit in this page and find the next write position. + // Set the bit in this page so we can find the next write position. valid |= 1 << writePos; // If we're full, we're done. @@ -224,9 +240,18 @@ public void takeFrom(InstancePage other) { this.valid.set(valid); other.valid.set(otherValid); + // If the other page was quite empty we may still be mergeable. mergeablePages.set(pageNo, isMergeable(valid)); - // With all assumptions in place we should have fully drained the other page, but check just to be safe. - mergeablePages.set(other.pageNo, isMergeable(otherValid)); + + // We definitely changed the contents and validity of this page. + contentsChanged.set(pageNo); + validityChanged.set(pageNo); + + // The other page will end up empty, so the validity changes and it's no longer mergeable. + // Also clear the changed bit so we don't re-upload the instances. + contentsChanged.clear(other.pageNo); + validityChanged.set(other.pageNo); + mergeablePages.clear(other.pageNo); if (isFull(valid)) { fullPages.set(pageNo); @@ -246,7 +271,7 @@ public void update(int modelIndex, int baseInstance) { this.baseInstance = baseInstance; var sameModelIndex = this.modelIndex == modelIndex; - if (sameModelIndex && changedPages.isEmpty()) { + if (sameModelIndex && validityChanged.isEmpty()) { // Nothing to do! return; } @@ -258,7 +283,7 @@ public void update(int modelIndex, int baseInstance) { if (sameModelIndex) { // Only need to update the changed pages. - for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + for (int page = validityChanged.nextSetBit(0); page >= 0 && page < pages.length; page = validityChanged.nextSetBit(page + 1)) { mapping.updatePage(page, modelIndex, pages[page].valid.get()); } } else { @@ -267,6 +292,8 @@ public void update(int modelIndex, int baseInstance) { mapping.updatePage(i, modelIndex, pages[i].valid.get()); } } + + validityChanged.clear(); } public void writeModel(long ptr) { @@ -280,15 +307,21 @@ public void writeModel(long ptr) { } public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { - if (changedPages.isEmpty()) { + if (contentsChanged.isEmpty()) { return; } var pages = this.pages.get(); - for (int page = changedPages.nextSetBit(0); page >= 0 && page < pages.length; page = changedPages.nextSetBit(page + 1)) { + for (int page = contentsChanged.nextSetBit(0); page >= 0 && page < pages.length; page = contentsChanged.nextSetBit(page + 1)) { var instances = pages[page].instances; long baseByte = mapping.page2ByteOffset(page); + + if (baseByte < 0) { + // This page is not mapped to the VBO. + continue; + } + long size = ObjectStorage.PAGE_SIZE * instanceStride; // Because writes are broken into pages, we end up with significantly more calls into @@ -321,7 +354,7 @@ public void uploadInstances(StagingBuffer stagingBuffer, int instanceVbo) { stagingBuffer.enqueueCopy(block.ptr(), size, instanceVbo, baseByte); } - changedPages.clear(); + contentsChanged.clear(); } public void parallelUpdate() { @@ -501,7 +534,7 @@ public int instanceCount() { */ public void clear() { this.pages.set(pageArray(0)); - changedPages.clear(); + contentsChanged.clear(); fullPages.clear(); } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java index 20b6ee87a..d7c7e67b3 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/ObjectStorage.java @@ -1,6 +1,7 @@ package dev.engine_room.flywheel.backend.engine.indirect; import java.util.Arrays; +import java.util.BitSet; import org.lwjgl.system.MemoryUtil; @@ -18,6 +19,7 @@ public class ObjectStorage extends AbstractArena { public static final int INITIAL_PAGES_ALLOCATED = 4; public static final int DESCRIPTOR_SIZE_BYTES = Integer.BYTES * 2; + private final BitSet changedFrames = new BitSet(); /** * The GPU side buffer containing all the objects, logically divided into page frames. */ @@ -31,8 +33,6 @@ public class ObjectStorage extends AbstractArena { */ private MemoryBlock frameDescriptors; - private boolean needsUpload = false; - public ObjectStorage(long objectSizeBytes) { super(PAGE_SIZE * objectSizeBytes); @@ -62,6 +62,16 @@ public void free(int i) { var ptr = ptrForPage(i); MemoryUtil.memPutInt(ptr, 0); MemoryUtil.memPutInt(ptr + 4, 0); + + changedFrames.set(i); + } + + private void set(int i, int modelIndex, int validBits) { + var ptr = ptrForPage(i); + MemoryUtil.memPutInt(ptr, modelIndex); + MemoryUtil.memPutInt(ptr + 4, validBits); + + changedFrames.set(i); } @Override @@ -72,12 +82,17 @@ protected void grow() { } public void uploadDescriptors(StagingBuffer stagingBuffer) { - if (!needsUpload) { + if (changedFrames.isEmpty()) { return; } - // We could be smarter about which spans are uploaded but this thing is so small it's probably not worth it. - stagingBuffer.enqueueCopy(frameDescriptors.ptr(), frameDescriptors.size(), frameDescriptorBuffer.handle(), 0); - needsUpload = false; + + var ptr = frameDescriptors.ptr(); + for (int i = changedFrames.nextSetBit(0); i >= 0 && i < capacity(); i = changedFrames.nextSetBit(i + 1)) { + var offset = (long) i * DESCRIPTOR_SIZE_BYTES; + stagingBuffer.enqueueCopy(ptr + offset, DESCRIPTOR_SIZE_BYTES, frameDescriptorBuffer.handle(), offset); + } + + changedFrames.clear(); } public void delete() { @@ -110,18 +125,14 @@ public void updatePage(int index, int modelIndex, int validBits) { holePunch(index); return; } - var page = pages[index]; + var frame = pages[index]; - if (page == INVALID_PAGE) { + if (frame == INVALID_PAGE) { // Un-holed punch. - page = unHolePunch(index); + frame = unHolePunch(index); } - var ptr = ptrForPage(page); - MemoryUtil.memPutInt(ptr, modelIndex); - MemoryUtil.memPutInt(ptr + 4, validBits); - - ObjectStorage.this.needsUpload = true; + ObjectStorage.this.set(frame, modelIndex, validBits); } /** @@ -132,8 +143,6 @@ public void updatePage(int index, int modelIndex, int validBits) { public void holePunch(int index) { ObjectStorage.this.free(pages[index]); pages[index] = INVALID_PAGE; - - ObjectStorage.this.needsUpload = true; } /** @@ -173,8 +182,6 @@ public void delete() { ObjectStorage.this.free(page); } pages = EMPTY_ALLOCATION; - - ObjectStorage.this.needsUpload = true; } private void grow(int neededPages, int oldLength) { From 540fe7a7feac03e20b2bd7c2ddb192427b175590 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sat, 2 Nov 2024 16:56:26 -0700 Subject: [PATCH 06/18] Acetaminophen - Generic pain relief - Use new Instance[] rather than capturing the class object of the instance type - Make InstancePage static, but manually track the instancer parent so we can check when stealing - Simplify array creation helpers and make them static - Mark InstanceHandleImpl#state as UnknownNullability --- .../backend/engine/InstanceHandleImpl.java | 5 +- .../engine/indirect/IndirectInstancer.java | 85 +++++++++---------- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java index 445f5dfba..e83936b0c 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/InstanceHandleImpl.java @@ -1,13 +1,16 @@ package dev.engine_room.flywheel.backend.engine; +import org.jetbrains.annotations.UnknownNullability; + import dev.engine_room.flywheel.api.instance.Instance; import dev.engine_room.flywheel.api.instance.InstanceHandle; public class InstanceHandleImpl implements InstanceHandle { + @UnknownNullability public State state; public int index; - public InstanceHandleImpl(State state) { + public InstanceHandleImpl(@UnknownNullability State state) { this.state = state; } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index d6a73d61e..d1f39907a 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -1,12 +1,10 @@ package dev.engine_room.flywheel.backend.engine.indirect; -import java.lang.reflect.Array; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.UnknownNullability; import org.joml.Vector4fc; @@ -26,7 +24,7 @@ public class IndirectInstancer extends AbstractInstancer private final List associatedDraws = new ArrayList<>(); private final Vector4fc boundingSphere; - private final AtomicReference pages; + private final AtomicReference[]> pages = new AtomicReference<>(pageArray(0)); /** * The set of pages whose count changed and thus need their descriptor re-uploaded. */ @@ -47,7 +45,6 @@ public class IndirectInstancer extends AbstractInstancer * but we also don't want to waste work merging into pages that are already empty. */ private final AtomicBitSet mergeablePages = new AtomicBitSet(); - private final Class instanceClass; public ObjectStorage.@UnknownNullability Mapping mapping; @@ -60,25 +57,25 @@ public IndirectInstancer(InstancerKey key, Recreate recreate) { .byteSize()); writer = this.type.writer(); boundingSphere = key.model().boundingSphere(); + } - instanceClass = (Class) type.create(new InstanceHandleImpl(null)) - .getClass(); - pages = new AtomicReference<>(pageArray(0)); + @SuppressWarnings("unchecked") + private static InstancePage[] pageArray(int length) { + return new InstancePage[length]; } - @NotNull @SuppressWarnings("unchecked") - private InstancePage[] pageArray(int length) { - return (InstancePage[]) Array.newInstance(InstancePage.class, length); + private static I[] instanceArray() { + return (I[]) new Instance[ObjectStorage.PAGE_SIZE]; } - @NotNull @SuppressWarnings("unchecked") - private I[] instanceArray() { - return (I[]) Array.newInstance(instanceClass, ObjectStorage.PAGE_SIZE); + private static InstanceHandleImpl[] handleArray() { + return new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; } - public final class InstancePage implements InstanceHandleImpl.State { + private static final class InstancePage implements InstanceHandleImpl.State { + private final IndirectInstancer parent; private final int pageNo; private final I[] instances; // Handles are only read in #takeFrom. It would be nice to avoid tracking these at all. @@ -88,17 +85,14 @@ public final class InstancePage implements InstanceHandleImpl.State { */ private final AtomicInteger valid; - InstancePage(int pageNo) { + private InstancePage(IndirectInstancer parent, int pageNo) { + this.parent = parent; this.pageNo = pageNo; this.instances = instanceArray(); - this.handles = (InstanceHandleImpl[]) new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; + this.handles = handleArray(); this.valid = new AtomicInteger(0); } - public int count() { - return Integer.bitCount(valid.get()); - } - /** * Attempt to add the given instance/handle to this page. * @@ -129,19 +123,19 @@ public boolean add(I instance, InstanceHandleImpl handle) { // Handle index is unique amongst all pages of this instancer. handle.index = local2HandleIndex(index); - contentsChanged.set(pageNo); - validityChanged.set(pageNo); + parent.contentsChanged.set(pageNo); + parent.validityChanged.set(pageNo); if (isFull(newValue)) { // The page is now full, mark it so in the bitset. // This is safe because only one bit position changes at a time. - fullPages.set(pageNo); + parent.fullPages.set(pageNo); } if (isEmpty(currentValue)) { // Value we just saw was zero, so since we added something we are now mergeable! - mergeablePages.set(pageNo); + parent.mergeablePages.set(pageNo); } else if (Integer.bitCount(currentValue) == 16) { // We just filled the 17th instance, so we are no longer mergeable. - mergeablePages.clear(pageNo); + parent.mergeablePages.clear(pageNo); } return true; } @@ -154,7 +148,7 @@ private int local2HandleIndex(int index) { @Override public InstanceHandleImpl.State setChanged(int index) { - contentsChanged.set(pageNo); + parent.contentsChanged.set(pageNo); return this; } @@ -170,16 +164,16 @@ public InstanceHandleImpl.State setDeleted(int index) { int newValue = currentValue & ~(1 << localIndex); if (valid.compareAndSet(currentValue, newValue)) { - validityChanged.set(pageNo); + parent.validityChanged.set(pageNo); if (isEmpty(newValue)) { // If we decremented to zero then we're no longer mergeable. - mergeablePages.clear(pageNo); + parent.mergeablePages.clear(pageNo); } else if (Integer.bitCount(newValue) == 16) { // If we decremented to 16 then we're now mergeable. - mergeablePages.set(pageNo); + parent.mergeablePages.set(pageNo); } // Set full page last so that other threads don't race to set the other bitsets. - fullPages.clear(pageNo); + parent.fullPages.clear(pageNo); return InstanceHandleImpl.Deleted.instance(); } } @@ -193,7 +187,7 @@ public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int int localIndex = index % ObjectStorage.PAGE_SIZE; - return new InstanceHandleImpl.Hidden<>(recreate, instances[localIndex]); + return new InstanceHandleImpl.Hidden<>(parent.recreate, instances[localIndex]); } /** @@ -201,7 +195,7 @@ public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int * * @param other The page to take instances from. */ - private void takeFrom(InstancePage other) { + private void takeFrom(InstancePage other) { // Fill the holes in this page with instances from the other page. int valid = this.valid.get(); @@ -241,20 +235,20 @@ private void takeFrom(InstancePage other) { other.valid.set(otherValid); // If the other page was quite empty we may still be mergeable. - mergeablePages.set(pageNo, isMergeable(valid)); + parent.mergeablePages.set(pageNo, isMergeable(valid)); // We definitely changed the contents and validity of this page. - contentsChanged.set(pageNo); - validityChanged.set(pageNo); + parent.contentsChanged.set(pageNo); + parent.validityChanged.set(pageNo); // The other page will end up empty, so the validity changes and it's no longer mergeable. // Also clear the changed bit so we don't re-upload the instances. - contentsChanged.clear(other.pageNo); - validityChanged.set(other.pageNo); - mergeablePages.clear(other.pageNo); + parent.contentsChanged.clear(other.pageNo); + parent.validityChanged.set(other.pageNo); + parent.mergeablePages.clear(other.pageNo); if (isFull(valid)) { - fullPages.set(pageNo); + parent.fullPages.set(pageNo); } } } @@ -456,8 +450,10 @@ public void stealInstance(@Nullable I instance) { // That seems kinda impossible so I'm fine leaving it as is for now. // Add the instance to this instancer. - if (handle.state instanceof InstancePage other) { - // TODO: shortcut here if we already own the instance + if (handle.state instanceof InstancePage other) { + if (other.parent == this) { + return; + } // Remove the instance from its old instancer. // This won't have any unwanted effect when the old instancer @@ -496,10 +492,10 @@ private void addInner(I instance, InstanceHandleImpl handle) { // Thread safety: segments contains all pages from the currently visible pages, plus extra. // all pages in the currently visible pages are canonical and will not change. // Can't just `new InstancePage[]` because it has a generic parameter. - InstancePage[] newPages = pageArray(desiredLength); + InstancePage[] newPages = pageArray(desiredLength); System.arraycopy(pages, 0, newPages, 0, pages.length); - newPages[pages.length] = new InstancePage(pages.length); + newPages[pages.length] = new InstancePage<>(this, pages.length); // because we are using a compareAndSet, if this thread "wins the race" and successfully sets this variable, then the new page becomes canonical. if (this.pages.compareAndSet(pages, newPages)) { @@ -535,7 +531,8 @@ public int instanceCount() { public void clear() { this.pages.set(pageArray(0)); contentsChanged.clear(); + validityChanged.clear(); fullPages.clear(); - + mergeablePages.clear(); } } From 4f0c1cc1ae2be5dc5315f7e3c360f75305ab0596 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sun, 3 Nov 2024 12:15:04 -0800 Subject: [PATCH 07/18] Checking the source - Remove source checks, it was very old and untouched - Remove FIXMEs and TODOs that were already fixed/done --- .../backend/compile/SourceChecks.java | 55 ------------------- .../component/InstanceAssemblerComponent.java | 2 +- .../flywheel/backend/engine/LightStorage.java | 2 - .../visualization/VisualizerRegistryImpl.java | 1 - .../flywheel/vanilla/VanillaVisuals.java | 20 ------- 5 files changed, 1 insertion(+), 79 deletions(-) delete mode 100644 common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java deleted file mode 100644 index fdfc5c3d1..000000000 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/SourceChecks.java +++ /dev/null @@ -1,55 +0,0 @@ -package dev.engine_room.flywheel.backend.compile; - -// TODO: recycle to be invoked by the shader compiler -public class SourceChecks { - // public static final BiConsumer LAYOUT_VERTEX = checkFunctionArity("flw_layoutVertex", 0); - // public static final BiConsumer INSTANCE_VERTEX = checkFunctionParameterTypeExists("flw_instanceVertex", 1, 0); - // public static final BiConsumer MATERIAL_VERTEX = checkFunctionArity("flw_materialVertex", 0); - // public static final BiConsumer MATERIAL_FRAGMENT = checkFunctionArity("flw_materialFragment", 0); - // public static final BiConsumer CONTEXT_VERTEX = checkFunctionArity("flw_contextVertex", 0); - // public static final BiConsumer CONTEXT_FRAGMENT = checkFunctionArity("flw_contextFragment", 0).andThen(checkFunctionArity("flw_initFragment", 0)); - // public static final BiConsumer PIPELINE = checkFunctionArity("main", 0); - // - // public static BiConsumer checkFunctionArity(String name, int arity) { - // return (errorReporter, file) -> checkFunctionArity(errorReporter, file, name, arity); - // } - // - // public static BiConsumer checkFunctionParameterTypeExists(String name, int arity, int param) { - // return (errorReporter, file) -> { - // var func = checkFunctionArity(errorReporter, file, name, arity); - // - // if (func == null) { - // return; - // } - // - // var maybeStruct = func.getParameterType(param) - // .findStruct(); - // - // if (maybeStruct.isEmpty()) { - // errorReporter.generateMissingStruct(file, func.getParameterType(param), "struct not defined"); - // } - // }; - // } - // - // /** - // * @return {@code null} if the function doesn't exist, or if the function has the wrong arity. - // */ - // @Nullable - // private static ShaderFunction checkFunctionArity(ErrorReporter errorReporter, SourceFile file, String name, int arity) { - // Optional maybeFunc = file.findFunction(name); - // - // if (maybeFunc.isEmpty()) { - // errorReporter.generateMissingFunction(file, name, "\"" + name + "\" function not defined"); - // return null; - // } - // - // ShaderFunction func = maybeFunc.get(); - // ImmutableList params = func.getParameters(); - // if (params.size() != arity) { - // errorReporter.generateFunctionArgumentCountError(name, arity, func.getArgs()); - // return null; - // } - // - // return func; - // } -} diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java index 7115e1a8b..eb42189b4 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/compile/component/InstanceAssemblerComponent.java @@ -62,7 +62,7 @@ public abstract class InstanceAssemblerComponent implements SourceComponent { FLOAT_UNPACKING_FUNCS.put(FloatRepr.UNSIGNED_INT, e -> e.cast("float")); FLOAT_UNPACKING_FUNCS.put(FloatRepr.NORMALIZED_UNSIGNED_INT, e -> e.cast("float").div(4294967295f)); - FLOAT_UNPACKING_FUNCS.put(FloatRepr.FLOAT, e -> e.callFunction("uintBitsToFloat")); // FIXME: GLSL 330+ + FLOAT_UNPACKING_FUNCS.put(FloatRepr.FLOAT, e -> e.callFunction("uintBitsToFloat")); } protected final Layout layout; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java index 47e884a52..5b5ac1d51 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java @@ -22,7 +22,6 @@ import net.minecraft.world.level.lighting.LayerLightEventListener; /** - * TODO: AO data * A managed arena of light sections for uploading to the GPU. * *

Each section represents an 18x18x18 block volume of light data. @@ -116,7 +115,6 @@ public Plan createFramePlan() { } // Now actually do the collection. - // TODO: Should this be done in parallel? sectionsToCollect.forEach(this::collectSection); updatedSections.clear(); diff --git a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java index b78562b0b..40522c89d 100644 --- a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java +++ b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/VisualizerRegistryImpl.java @@ -11,7 +11,6 @@ import net.minecraft.world.level.block.entity.BlockEntity; import net.minecraft.world.level.block.entity.BlockEntityType; -// TODO: Add freezing @SuppressWarnings("unchecked") public final class VisualizerRegistryImpl { @Nullable diff --git a/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java b/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java index 53efc4562..3516cd041 100644 --- a/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java +++ b/common/src/main/java/dev/engine_room/flywheel/vanilla/VanillaVisuals.java @@ -7,26 +7,6 @@ import net.minecraft.world.entity.EntityType; import net.minecraft.world.level.block.entity.BlockEntityType; -/** - * TODO: - * - * - * - * - * - * - * - * - * - * - * - * - * - * - * - * - *
{@link BlockEntityType#SIGN} {@link net.minecraft.client.renderer.blockentity.SignRenderer SignRenderer}
{@link BlockEntityType#PISTON} {@link net.minecraft.client.renderer.blockentity.PistonHeadRenderer PistonHeadRenderer}
{@link BlockEntityType#CONDUIT} {@link net.minecraft.client.renderer.blockentity.ConduitRenderer ConduitRenderer}
{@link BlockEntityType#ENCHANTING_TABLE} {@link net.minecraft.client.renderer.blockentity.EnchantTableRenderer EnchantTableRenderer}
{@link BlockEntityType#LECTERN} {@link net.minecraft.client.renderer.blockentity.LecternRenderer LecternRenderer}
{@link BlockEntityType#MOB_SPAWNER} {@link net.minecraft.client.renderer.blockentity.SpawnerRenderer SpawnerRenderer}
{@link BlockEntityType#BED} {@link net.minecraft.client.renderer.blockentity.BedRenderer BedRenderer}
^^ Interesting - Major vv
{@link BlockEntityType#END_PORTAL} {@link net.minecraft.client.renderer.blockentity.TheEndPortalRenderer TheEndPortalRenderer}
{@link BlockEntityType#END_GATEWAY} {@link net.minecraft.client.renderer.blockentity.TheEndGatewayRenderer TheEndGatewayRenderer}
{@link BlockEntityType#BEACON} {@link net.minecraft.client.renderer.blockentity.BeaconRenderer BeaconRenderer}
{@link BlockEntityType#SKULL} {@link net.minecraft.client.renderer.blockentity.SkullBlockRenderer SkullBlockRenderer}
{@link BlockEntityType#BANNER} {@link net.minecraft.client.renderer.blockentity.BannerRenderer BannerRenderer}
{@link BlockEntityType#STRUCTURE_BLOCK} {@link net.minecraft.client.renderer.debug.StructureRenderer StructureRenderer}
{@link BlockEntityType#CAMPFIRE} {@link net.minecraft.client.renderer.blockentity.CampfireRenderer CampfireRenderer}
- */ public class VanillaVisuals { public static void init() { builder(BlockEntityType.CHEST) From 461578ec0ea44eb4736ed7a3ba20fb273eba3c78 Mon Sep 17 00:00:00 2001 From: IThundxr Date: Sun, 3 Nov 2024 20:57:47 -0500 Subject: [PATCH 08/18] Automated testing (#269) * Automated testing * Testing testing - Use 2 spaces for indents yaml - Move setupTestMod to PlatformExtension - Allow specifying the sourceSet for the testMod artifact - Rename things to camelCase - Use rootCompile from transitiveSourceSets for the testMod source sets - Use a blanket remapTestModJar task in the gh actions build * Fail slowly - We want to know the results of both tests regardless * Add workflow dispatch * Shoes should be steel toed, dangerous stuff * Update build.yml * fix modid * Update FlywheelTestModClient.java * add debug logging * fix syntax issues * fix issues * Update build.yml * Add debug logging * more logging * get testmod from correct dir * switch to env var * Why wait? - Immediately audit on client tick * DidObfuscate - Fix RenderSystemMixin on fabric - setShaderFogShape's arguments need to be remapped, but the name of the function should not be. Fortunately mixin allows matching by function name alone * Clever commit title - Change the Fabric mod ID to match Forge - Move "Flywheel Test Mod" to static - Cleanup start/stop messages - Use the client start event on Fabric --------- Co-authored-by: Jozufozu --- .editorconfig | 3 + .github/workflows/build.yml | 102 ++++++++++++------ .../gradle/platform/PlatformExtension.kt | 31 +++++- .../backend/mixin/RenderSystemMixin.java | 5 +- fabric/build.gradle.kts | 5 + .../flywheel/FlywheelTestModClient.java | 29 +++++ fabric/src/testMod/resources/fabric.mod.json | 13 +++ forge/build.gradle.kts | 5 + .../flywheel/FlywheelTestModClient.java | 33 ++++++ .../src/testMod/resources/META-INF/mods.toml | 8 ++ 10 files changed, 197 insertions(+), 37 deletions(-) create mode 100644 fabric/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java create mode 100644 fabric/src/testMod/resources/fabric.mod.json create mode 100644 forge/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java create mode 100644 forge/src/testMod/resources/META-INF/mods.toml diff --git a/.editorconfig b/.editorconfig index 038bc765e..294e122e9 100644 --- a/.editorconfig +++ b/.editorconfig @@ -9,6 +9,9 @@ charset = utf-8 trim_trailing_whitespace = true insert_final_newline = true +[*.yml] +indent_size = 2 + [*.json] indent_size = 2 max_line_length = 500 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index dd1169eae..cf5b1c501 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,47 +1,85 @@ -name: build +name: Build -on: [ pull_request, push ] +on: [ workflow_dispatch, pull_request, push ] + +env: + JAVA_VERSION: 17 jobs: build: - strategy: - matrix: - java: [ - 17 # Current Java LTS & minimum supported by Minecraft - ] - os: [ ubuntu-latest ] - runs-on: ${{ matrix.os }} + runs-on: ubuntu-latest steps: - - name: Checkout + - name: Checkout Repository uses: actions/checkout@v4 - - name: Validate Gradle Wrapper - uses: gradle/actions/wrapper-validation@v3 - - name: Gradle Cache + + - name: Setup Java + run: echo "JAVA_HOME=$JAVA_HOME_${{ env.JAVA_VERSION }}_X64" >> "$GITHUB_ENV" + + - name: Loom Cache uses: actions/cache@v4 with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - .gradle/loom-cache - build/ - key: ${{ runner.os }}-jdk${{ matrix.java }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle.properties', '**/gradle-wrapper.properties', '.github/workflows/build.yml') }} - - name: Setup JDK ${{ matrix.java }} - uses: actions/setup-java@v4 + path: "**/.gradle/loom-cache" + key: "${{ runner.os }}-gradle-${{ hashFiles('**/libs.versions.*', '**/*.gradle*', '**/gradle-wrapper.properties') }}" + restore-keys: "${{ runner.os }}-gradle-" + + - name: Setup Gradle + uses: gradle/actions/setup-gradle@v3 with: - distribution: 'temurin' - java-version: ${{ matrix.java }} - - name: Make Gradle Wrapper Executable - if: ${{ runner.os != 'Windows' }} - run: chmod +x ./gradlew + gradle-home-cache-cleanup: true + cache-read-only: ${{ !endsWith(github.ref_name, '/dev') }} + + - name: Validate Gradle Wrapper Integrity + uses: gradle/wrapper-validation-action@v2 + - name: Build - # doesn't actually publish, as no secrets are passed in, just makes sure that publishing works - run: ./gradlew publish --no-daemon + # Doesn't actually publish, as no secrets are passed in, just makes sure that publishing works + # Also generate the mod jars for the test job + run: ./gradlew remapTestModJar publish --no-daemon + - name: Capture Build Artifacts - if: ${{ runner.os == 'Linux' && matrix.java == '17' }} uses: actions/upload-artifact@v4 with: name: Artifacts path: | - common/build/libs/ - fabric/build/libs/ - forge/build/libs/ + common/build/libs/ + fabric/build/libs/ + forge/build/libs/ + + test: + strategy: + fail-fast: false + matrix: + loader: [ forge, fabric ] + needs: build + runs-on: ubuntu-latest + steps: + - name: Checkout Repository + uses: actions/checkout@v4 + + - name: Download build artifact + uses: actions/download-artifact@v4 + with: + name: Artifacts + + - name: Setup Environment Variables + run: | + echo "MOD_VERSION=$(grep '^mod_version =' gradle.properties | cut -d'=' -f2 | tr -d ' ')" >> "$GITHUB_ENV" + echo "MINECRAFT_VERSION=$(grep '^minecraft_version =' gradle.properties | cut -d'=' -f2 | tr -d ' ')" >> "$GITHUB_ENV" + echo "FABRIC_API_VERSION=$(grep '^fabric_api_version =' gradle.properties | cut -d'=' -f2 | tr -d ' ' | sed 's/+.*//')" >> "$GITHUB_ENV" + + - name: Move Test Mod and Flywheel into run/mods + run: | + mkdir -p run/mods + cp ${{ matrix.loader }}/build/libs/flywheel-${{ matrix.loader }}-${{ env.MINECRAFT_VERSION }}-${{ env.MOD_VERSION }}.jar run/mods + cp ${{ matrix.loader }}/build/libs/flywheel-${{ matrix.loader }}-${{ env.MINECRAFT_VERSION }}-${{ env.MOD_VERSION }}-testmod.jar run/mods + + # Lock to a specific commit, it would be bad if the tag is re-pushed with unwanted changes + - name: Run the MC client + uses: 3arthqu4ke/mc-runtime-test@e72f8fe1134aabf6fc749a2a8c09bb56dd7d283e + with: + mc: ${{ env.MINECRAFT_VERSION }} + modloader: ${{ matrix.loader }} + regex: .*${{ matrix.loader }}.* + mc-runtime-test: none + java: ${{ env.JAVA_VERSION }} + fabric-api: ${{ matrix.loader == 'fabric' && env.FABRIC_API_VERSION || 'none' }} diff --git a/buildSrc/src/main/kotlin/dev/engine_room/gradle/platform/PlatformExtension.kt b/buildSrc/src/main/kotlin/dev/engine_room/gradle/platform/PlatformExtension.kt index 66831cedf..2addef1dc 100644 --- a/buildSrc/src/main/kotlin/dev/engine_room/gradle/platform/PlatformExtension.kt +++ b/buildSrc/src/main/kotlin/dev/engine_room/gradle/platform/PlatformExtension.kt @@ -2,17 +2,17 @@ package dev.engine_room.gradle.platform import dev.engine_room.gradle.jarset.JarTaskSet import net.fabricmc.loom.api.LoomGradleExtensionAPI +import net.fabricmc.loom.task.RemapJarTask import org.gradle.api.Project +import org.gradle.api.Task import org.gradle.api.tasks.SourceSet import org.gradle.api.tasks.SourceSetContainer import org.gradle.api.tasks.compile.JavaCompile import org.gradle.api.tasks.javadoc.Javadoc import org.gradle.jvm.tasks.Jar -import org.gradle.kotlin.dsl.named -import org.gradle.kotlin.dsl.provideDelegate -import org.gradle.kotlin.dsl.the -import org.gradle.kotlin.dsl.withType +import org.gradle.kotlin.dsl.* import org.gradle.language.jvm.tasks.ProcessResources +import java.io.File import kotlin.properties.ReadWriteProperty import kotlin.reflect.KProperty @@ -102,6 +102,29 @@ open class PlatformExtension(val project: Project) { } } + fun setupTestMod(sourceSet: SourceSet) { + project.tasks.apply { + val testModJar = register("testModJar") { + from(sourceSet.output) + val file = File(project.layout.buildDirectory.asFile.get(), "devlibs"); + destinationDirectory.set(file) + archiveClassifier = "testmod" + } + + val remapTestModJar = register("remapTestModJar") { + dependsOn(testModJar) + inputFile.set(testModJar.get().archiveFile) + archiveClassifier = "testmod" + addNestedDependencies = false + classpath.from(sourceSet.compileClasspath) + } + + named("build").configure { + dependsOn(remapTestModJar) + } + } + } + private class DependentProject(private val thisProject: Project) : ReadWriteProperty { private var value: Project? = null diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/mixin/RenderSystemMixin.java b/common/src/backend/java/dev/engine_room/flywheel/backend/mixin/RenderSystemMixin.java index 9a51df11b..b08444dc7 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/mixin/RenderSystemMixin.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/mixin/RenderSystemMixin.java @@ -27,7 +27,10 @@ abstract class RenderSystemMixin { FogUniforms.update(); } - @Inject(method = "setShaderFogShape(Lcom/mojang/blaze3d/shaders/FogShape;)V", at = @At("RETURN")) + // Fabric fails to resolve the mixin in prod when the full signature is specified. + // I suspect it's because this method references a class name in its signature, + // and that needs to be remapped while the function names in RenderSystem are marked with @DontObfuscate. + @Inject(method = "setShaderFogShape", at = @At("RETURN")) private static void flywheel$onSetFogShape(CallbackInfo ci) { FogUniforms.update(); } diff --git a/fabric/build.gradle.kts b/fabric/build.gradle.kts index 865773be5..b1b04dad8 100644 --- a/fabric/build.gradle.kts +++ b/fabric/build.gradle.kts @@ -12,6 +12,7 @@ val lib = sourceSets.create("lib") val backend = sourceSets.create("backend") val stubs = sourceSets.create("stubs") val main = sourceSets.getByName("main") +val testMod = sourceSets.create("testMod") transitiveSourceSets { compileClasspath = main.compileClasspath @@ -35,6 +36,9 @@ transitiveSourceSets { compile(stubs) implementation(api, lib, backend) } + sourceSet(testMod) { + rootCompile() + } createCompileConfigurations() } @@ -45,6 +49,7 @@ platform { setupLoomMod(api, lib, backend, main) setupLoomRuns() setupFatJar(api, lib, backend, main) + setupTestMod(testMod) } jarSets { diff --git a/fabric/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java b/fabric/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java new file mode 100644 index 000000000..6b05b1e62 --- /dev/null +++ b/fabric/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java @@ -0,0 +1,29 @@ +package dev.engine_room.flywheel; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.spongepowered.asm.mixin.MixinEnvironment; + +import net.fabricmc.api.ClientModInitializer; +import net.fabricmc.fabric.api.client.event.lifecycle.v1.ClientLifecycleEvents; +import net.fabricmc.loader.api.FabricLoader; + +public class FlywheelTestModClient implements ClientModInitializer { + public static final String NAME = "Flywheel Test Mod"; + private static final Logger LOGGER = LoggerFactory.getLogger(NAME); + + @Override + public void onInitializeClient() { + LOGGER.info("Starting {} on EnvType: {}", NAME, FabricLoader.getInstance() + .getEnvironmentType()); + + ClientLifecycleEvents.CLIENT_STARTED.register(client -> { + LOGGER.info("Running mixin audit"); + MixinEnvironment.getCurrentEnvironment() + .audit(); + + LOGGER.info("Stopping client"); + client.stop(); + }); + } +} diff --git a/fabric/src/testMod/resources/fabric.mod.json b/fabric/src/testMod/resources/fabric.mod.json new file mode 100644 index 000000000..11bd545a1 --- /dev/null +++ b/fabric/src/testMod/resources/fabric.mod.json @@ -0,0 +1,13 @@ +{ + "schemaVersion": 1, + "id" : "${mod_id}_testmod", + "name": "${mod_name} Test Mod", + "version": "1.0.0", + "environment": "*", + "license": "${mod_license}", + "entrypoints": { + "client": [ + "dev.engine_room.flywheel.FlywheelTestModClient" + ] + } +} diff --git a/forge/build.gradle.kts b/forge/build.gradle.kts index d1c6b3617..6900fd197 100644 --- a/forge/build.gradle.kts +++ b/forge/build.gradle.kts @@ -12,6 +12,7 @@ val lib = sourceSets.create("lib") val backend = sourceSets.create("backend") val stubs = sourceSets.create("stubs") val main = sourceSets.getByName("main") +val testMod = sourceSets.create("testMod") transitiveSourceSets { compileClasspath = main.compileClasspath @@ -33,6 +34,9 @@ transitiveSourceSets { sourceSet(main) { compile(api, lib, backend, stubs) } + sourceSet(testMod) { + rootCompile() + } createCompileConfigurations() } @@ -43,6 +47,7 @@ platform { setupLoomMod(api, lib, backend, main) setupLoomRuns() setupFatJar(api, lib, backend, main) + setupTestMod(testMod) } jarSets { diff --git a/forge/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java b/forge/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java new file mode 100644 index 000000000..f04eb604f --- /dev/null +++ b/forge/src/testMod/java/dev/engine_room/flywheel/FlywheelTestModClient.java @@ -0,0 +1,33 @@ +package dev.engine_room.flywheel; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.spongepowered.asm.mixin.MixinEnvironment; + +import net.minecraft.client.Minecraft; +import net.minecraftforge.common.MinecraftForge; +import net.minecraftforge.event.TickEvent; +import net.minecraftforge.fml.common.Mod; +import net.minecraftforge.fml.loading.FMLLoader; + +@Mod("flywheel_testmod") +public class FlywheelTestModClient { + public static final String NAME = "Flywheel Test Mod"; + private static final Logger LOGGER = LoggerFactory.getLogger(NAME); + + public FlywheelTestModClient() { + LOGGER.info("Starting {} on Dist: {}", NAME, FMLLoader.getDist()); + + MinecraftForge.EVENT_BUS.addListener((TickEvent.ClientTickEvent e) -> { + if (e.phase == TickEvent.Phase.END) { + LOGGER.info("Running mixin audit"); + MixinEnvironment.getCurrentEnvironment() + .audit(); + + LOGGER.info("Stopping client"); + Minecraft.getInstance() + .stop(); + } + }); + } +} diff --git a/forge/src/testMod/resources/META-INF/mods.toml b/forge/src/testMod/resources/META-INF/mods.toml new file mode 100644 index 000000000..4d76a503e --- /dev/null +++ b/forge/src/testMod/resources/META-INF/mods.toml @@ -0,0 +1,8 @@ +modLoader = "javafml" +loaderVersion = "[0,)" +license = "${mod_license}" + +[[mods]] +modId = "${mod_id}_testmod" +version = "1.0.0" +displayName = "${mod_name} Test Mod" From e6fecc60b5da38a2046b4758a66b6786bc64cef7 Mon Sep 17 00:00:00 2001 From: IThundxr Date: Fri, 8 Nov 2024 12:05:50 -0500 Subject: [PATCH 09/18] Remove rubidium optional dependency (#270) --- forge/src/main/resources/META-INF/mods.toml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/forge/src/main/resources/META-INF/mods.toml b/forge/src/main/resources/META-INF/mods.toml index be991ac88..766bbfe9f 100644 --- a/forge/src/main/resources/META-INF/mods.toml +++ b/forge/src/main/resources/META-INF/mods.toml @@ -26,13 +26,6 @@ mandatory = true versionRange = "${forge_version_range}" side = "CLIENT" -# Simulates a breaks/incompatible dependency -[[dependencies.${mod_id}]] -modId = "rubidium" -mandatory = false -versionRange = "[0.0-INCOMPATIBLE]" -side = "CLIENT" - [[dependencies.${mod_id}]] modId = "embeddium" mandatory = false From 376ac76ac25ecf31de400ed18c21bd4adcad0738 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 8 Nov 2024 09:32:02 -0800 Subject: [PATCH 10/18] A whole lut of refactors - Replace monolithic lut building function with a class representing a layer of the lut - Actually need 2 classes because int[] and Object[] aren't trivial to make a type parameter, and we don't really want to be boxing ints here - No longer need sorted inputs - Should fix index out of bounds crash caused by reserving space for the wrong index layer - This will make it much easier to change the coordinate ordering scheme --- .../flywheel/backend/engine/LightLut.java | 222 ++++++++++-------- 1 file changed, 127 insertions(+), 95 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java index bb04c7760..d17ed806d 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java @@ -1,32 +1,34 @@ package dev.engine_room.flywheel.backend.engine; -import org.jetbrains.annotations.NotNull; +import java.util.function.BiConsumer; +import java.util.function.Supplier; import it.unimi.dsi.fastutil.ints.IntArrayList; -import it.unimi.dsi.fastutil.ints.IntObjectImmutablePair; -import it.unimi.dsi.fastutil.ints.IntObjectPair; import it.unimi.dsi.fastutil.longs.Long2IntMap; -import it.unimi.dsi.fastutil.longs.LongArrayList; -import it.unimi.dsi.fastutil.longs.LongComparator; -import it.unimi.dsi.fastutil.objects.ReferenceArrayList; import net.minecraft.core.SectionPos; public final class LightLut { - private static final LongComparator SECTION_X_THEN_Y_THEN_Z = (long a, long b) -> { - final var xComp = Integer.compare(SectionPos.x(a), SectionPos.x(b)); - if (xComp != 0) { - return xComp; - } - var yComp = Integer.compare(SectionPos.y(a), SectionPos.y(b)); - if (yComp != 0) { - return yComp; - } - return Integer.compare(SectionPos.z(a), SectionPos.z(b)); - }; + private final Layer> indices = new Layer<>(); private LightLut() { } + private void add(long position, int index) { + final var x = SectionPos.x(position); + final var y = SectionPos.y(position); + final var z = SectionPos.z(position); + + indices.computeIfAbsent(x, Layer::new) + .computeIfAbsent(y, IntLayer::new) + .set(z, index + 1); + } + + private IntArrayList toLut() { + final var out = new IntArrayList(); + indices.fillLut(out, (yIndices, lut) -> yIndices.fillLut(lut, IntLayer::fillLut)); + return out; + } + // Massive kudos to RogueLogix for figuring out this LUT scheme. // TODO: switch to y x z or x z y ordering // DATA LAYOUT @@ -38,105 +40,135 @@ public static IntArrayList buildLut(Long2IntMap sectionIndicesMaps) { if (sectionIndicesMaps.isEmpty()) { return new IntArrayList(); } - final var positions = sortedKeys(sectionIndicesMaps); - final var baseX = SectionPos.x(positions.getLong(0)); - return buildLut(baseX, buildIndices(sectionIndicesMaps, positions, baseX)); + var out = new LightLut(); + + sectionIndicesMaps.forEach(out::add); + + return out.toLut(); } - private static ReferenceArrayList>> buildIndices(Long2IntMap sectionIndicesMaps, LongArrayList positions, int baseX) { - final var indices = new ReferenceArrayList>>(); - for (long position : positions) { - final var x = SectionPos.x(position); - final var y = SectionPos.y(position); - final var z = SectionPos.z(position); - - final var xIndex = x - baseX; - if (indices.size() <= xIndex) { - indices.ensureCapacity(xIndex + 1); - indices.size(xIndex + 1); + private static final class Layer { + private boolean hasBase = false; + private int base = 0; + private Object[] nextLayer = new Object[0]; + + public void fillLut(IntArrayList lut, BiConsumer inner) { + lut.add(base); + lut.add(nextLayer.length); + + int innerIndexBase = lut.size(); + + // Reserve space for the inner indices... + lut.size(innerIndexBase + nextLayer.length); + + for (int i = 0; i < nextLayer.length; i++) { + final var innerIndices = (T) nextLayer[i]; + if (innerIndices == null) { + continue; + } + + int layerPosition = lut.size(); + + // ...so we can write in their actual positions later. + lut.set(innerIndexBase + i, layerPosition); + + // Append the next layer to the lut. + inner.accept(innerIndices, lut); } - var yLookup = indices.get(xIndex); - if (yLookup == null) { - //noinspection SuspiciousNameCombination - yLookup = new IntObjectImmutablePair<>(y, new ReferenceArrayList<>()); - indices.set(xIndex, yLookup); + } + + public T computeIfAbsent(int i, Supplier ifAbsent) { + if (!hasBase) { + // We don't want to default to base 0, so we'll use the first value we get. + base = i; + hasBase = true; } - final var yIndices = yLookup.right(); - final var yIndex = y - yLookup.leftInt(); - if (yIndices.size() <= yIndex) { - yIndices.ensureCapacity(yIndex + 1); - yIndices.size(yIndex + 1); + if (i < base) { + rebase(i); } - var zLookup = yIndices.get(yIndex); - if (zLookup == null) { - zLookup = new IntArrayList(); - zLookup.add(z); - zLookup.add(0); // this value will be filled in later - yIndices.set(yIndex, zLookup); + + final var offset = i - base; + + if (offset >= nextLayer.length) { + resize(offset + 1); } - final var zIndex = z - zLookup.getInt(0); - if ((zLookup.size() - 2) <= zIndex) { - zLookup.ensureCapacity(zIndex + 3); - zLookup.size(zIndex + 3); + var out = nextLayer[offset]; + + if (out == null) { + out = ifAbsent.get(); + nextLayer[offset] = out; } - // Add 1 to the actual index so that 0 indicates a missing section. - zLookup.set(zIndex + 2, sectionIndicesMaps.get(position) + 1); + return (T) out; } - return indices; - } - private static @NotNull LongArrayList sortedKeys(Long2IntMap sectionIndicesMaps) { - final var out = new LongArrayList(sectionIndicesMaps.keySet()); - out.unstableSort(SECTION_X_THEN_Y_THEN_Z); - return out; + private void resize(int length) { + final var newIndices = new Object[length]; + System.arraycopy(nextLayer, 0, newIndices, 0, nextLayer.length); + nextLayer = newIndices; + } + + private void rebase(int newBase) { + final var growth = base - newBase; + + final var newIndices = new Object[nextLayer.length + growth]; + // Shift the existing elements to the end of the new array to maintain their offset with the new base. + System.arraycopy(nextLayer, 0, newIndices, growth, nextLayer.length); + + nextLayer = newIndices; + base = newBase; + } } - private static IntArrayList buildLut(int baseX, ReferenceArrayList>> indices) { - final var out = new IntArrayList(); - out.add(baseX); - out.add(indices.size()); - for (int i = 0; i < indices.size(); i++) { - out.add(0); + private static final class IntLayer { + private boolean hasBase = false; + private int base = 0; + private int[] indices = new int[0]; + + public void fillLut(IntArrayList lut) { + lut.add(base); + lut.add(indices.length); + + for (int index : indices) { + lut.add(index); + } } - for (int x = 0; x < indices.size(); x++) { - final var yLookup = indices.get(x); - if (yLookup == null) { - out.set(x + 2, 0); - continue; + + public void set(int i, int index) { + if (!hasBase) { + base = i; + hasBase = true; } - // ensure that the base position and size dont cross a (64 byte) cache line - if ((out.size() & 0xF) == 0xF) { - out.add(0); + + if (i < base) { + rebase(i); } - final var baseYIndex = out.size(); - out.set(x + 2, baseYIndex); + final var offset = i - base; - final var yIndices = yLookup.right(); - out.add(yLookup.leftInt()); - out.add(yIndices.size()); - for (int i = 0; i < indices.size(); i++) { - out.add(0); + if (offset >= indices.length) { + resize(offset + 1); } - for (int y = 0; y < yIndices.size(); y++) { - final var zLookup = yIndices.get(y); - if (zLookup == null) { - out.set(baseYIndex + y + 2, 0); - continue; - } - // ensure that the base position and size dont cross a (64 byte) cache line - if ((out.size() & 0xF) == 0xF) { - out.add(0); - } - out.set(baseYIndex + y + 2, out.size()); - zLookup.set(1, zLookup.size() - 2); - out.addAll(zLookup); - } + indices[offset] = index; + } + + private void resize(int length) { + final var newIndices = new int[length]; + System.arraycopy(indices, 0, newIndices, 0, indices.length); + indices = newIndices; + } + + private void rebase(int newBase) { + final var growth = base - newBase; + + final var newIndices = new int[indices.length + growth]; + System.arraycopy(indices, 0, newIndices, growth, indices.length); + + indices = newIndices; + base = newBase; } - return out; } } From c8d76c32a7880e59ed4433a50af9c961b87aee8d Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 8 Nov 2024 09:58:40 -0800 Subject: [PATCH 11/18] Tightening the screws - Fix hiz test bounds clamping, good ol off by 1 error - Remove check for useMin since it's never used and the depth reduce shader is hard-coded already --- .../assets/flywheel/flywheel/internal/indirect/cull.glsl | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl index 1869e33b8..96ed0cd81 100644 --- a/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl +++ b/common/src/backend/resources/assets/flywheel/flywheel/internal/indirect/cull.glsl @@ -102,19 +102,14 @@ bool _flw_isVisible(uint instanceIndex, uint modelIndex) { // Clamp to the texture bounds. // Since we're not going through a sampler out of bounds texel fetches will return 0. - bounds = clamp(bounds, ivec4(0), levelSizePair); + bounds = clamp(bounds, ivec4(0), levelSizePair - ivec4(1)); float depth01 = texelFetch(_flw_depthPyramid, bounds.xw, level).r; float depth11 = texelFetch(_flw_depthPyramid, bounds.zw, level).r; float depth10 = texelFetch(_flw_depthPyramid, bounds.zy, level).r; float depth00 = texelFetch(_flw_depthPyramid, bounds.xy, level).r; - float depth; - if (_flw_cullData.useMin == 0) { - depth = max(max(depth00, depth01), max(depth10, depth11)); - } else { - depth = min(min(depth00, depth01), min(depth10, depth11)); - } + float depth = max(max(depth00, depth01), max(depth10, depth11)); float depthSphere = 1. + _flw_cullData.znear / (center.z + radius); From eae1d0ef9414117ec20eabc1f33668c22ad66eef Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 8 Nov 2024 10:13:29 -0800 Subject: [PATCH 12/18] The 3 time world heavyweight champion - Fix instance hiding on indirect --- .../engine/indirect/IndirectInstancer.java | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index d1f39907a..5e182a663 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -156,6 +156,27 @@ public InstanceHandleImpl.State setChanged(int index) { public InstanceHandleImpl.State setDeleted(int index) { int localIndex = index % ObjectStorage.PAGE_SIZE; + clear(localIndex); + + return InstanceHandleImpl.Deleted.instance(); + } + + @Override + public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { + if (visible) { + return this; + } + + int localIndex = index % ObjectStorage.PAGE_SIZE; + + var out = instances[localIndex]; + + clear(localIndex); + + return new InstanceHandleImpl.Hidden<>(parent.recreate, out); + } + + private void clear(int localIndex) { instances[localIndex] = null; handles[localIndex] = null; @@ -174,22 +195,11 @@ public InstanceHandleImpl.State setDeleted(int index) { } // Set full page last so that other threads don't race to set the other bitsets. parent.fullPages.clear(pageNo); - return InstanceHandleImpl.Deleted.instance(); + break; } } } - @Override - public InstanceHandleImpl.State setVisible(InstanceHandleImpl handle, int index, boolean visible) { - if (visible) { - return this; - } - - int localIndex = index % ObjectStorage.PAGE_SIZE; - - return new InstanceHandleImpl.Hidden<>(parent.recreate, instances[localIndex]); - } - /** * Only call this on 2 pages that are mergeable. * From 68453d83498940943dad115b5c07675c15bedf99 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 8 Nov 2024 16:10:44 -0800 Subject: [PATCH 13/18] Always the first thing to fall apart - Fix crumbling on indirect --- .../flywheel/backend/engine/DrawManager.java | 20 +++++++++++-------- .../engine/indirect/IndirectDrawManager.java | 2 +- .../engine/indirect/IndirectInstancer.java | 8 ++++++++ .../instancing/InstancedDrawManager.java | 10 +++++++++- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java index 898253bb0..b81502d1f 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java @@ -8,6 +8,8 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; +import org.jetbrains.annotations.Nullable; + import com.mojang.datafixers.util.Pair; import dev.engine_room.flywheel.api.RenderContext; @@ -106,7 +108,13 @@ private static boolean checkAndWarnEmptyModel(Model model) { return false; } - protected static > Map, Int2ObjectMap>>>> doCrumblingSort(Class clazz, List crumblingBlocks) { + @FunctionalInterface + protected interface State2Instancer> { + // I tried using a plain Function, I> here, but it exploded with type errors. + @Nullable I apply(InstanceHandleImpl.State state); + } + + protected static > Map, Int2ObjectMap>>>> doCrumblingSort(List crumblingBlocks, State2Instancer cast) { Map, Int2ObjectMap>>>> byType = new HashMap<>(); for (Engine.CrumblingBlock block : crumblingBlocks) { int progress = block.progress(); @@ -123,16 +131,12 @@ protected static > Map, Int2ObjectMap continue; } - InstanceHandleImpl.State abstractInstancer = impl.state; - // AbstractInstancer directly implement HandleState, so this check is valid. - if (!clazz.isInstance(abstractInstancer)) { - // This rejects instances that were created by a different engine, - // and also instances that are hidden or deleted. + var instancer = cast.apply(impl.state); + + if (instancer == null) { continue; } - var instancer = clazz.cast(abstractInstancer); - byType.computeIfAbsent(new GroupKey<>(instancer.type, instancer.environment), $ -> new Int2ObjectArrayMap<>()) .computeIfAbsent(progress, $ -> new ArrayList<>()) .add(Pair.of(instancer, impl)); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java index 4b14df6b2..8a6dd5e4b 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java @@ -189,7 +189,7 @@ public void delete() { } public void renderCrumbling(List crumblingBlocks) { - var byType = doCrumblingSort(IndirectInstancer.class, crumblingBlocks); + var byType = doCrumblingSort(crumblingBlocks, IndirectInstancer::fromState); if (byType.isEmpty()) { return; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java index 5e182a663..1291e9a77 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectInstancer.java @@ -74,6 +74,14 @@ private static InstanceHandleImpl[] handleArray() { return new InstanceHandleImpl[ObjectStorage.PAGE_SIZE]; } + @Nullable + public static IndirectInstancer fromState(InstanceHandleImpl.State handle) { + if (handle instanceof InstancePage instancer) { + return instancer.parent; + } + return null; + } + private static final class InstancePage implements InstanceHandleImpl.State { private final IndirectInstancer parent; private final int pageNo; diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedDrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedDrawManager.java index 3d2cf0c76..88526f360 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedDrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/instancing/InstancedDrawManager.java @@ -147,7 +147,15 @@ protected void initialize(InstancerKey key, InstancedIns @Override public void renderCrumbling(List crumblingBlocks) { // Sort draw calls into buckets, so we don't have to do as many shader binds. - var byType = doCrumblingSort(InstancedInstancer.class, crumblingBlocks); + var byType = doCrumblingSort(crumblingBlocks, handle -> { + // AbstractInstancer directly implement HandleState, so this check is valid. + if (handle instanceof InstancedInstancer instancer) { + return instancer; + } + // This rejects instances that were created by a different engine, + // and also instances that are hidden or deleted. + return null; + }); if (byType.isEmpty()) { return; From 5b97a56c8cf8a58930c4e101011e89d276041005 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Fri, 8 Nov 2024 16:13:28 -0800 Subject: [PATCH 14/18] Save yourself the trouble - Don't initialize instancers if they have 0 instances - Never shrink the index pool - Actually process recently allocated meshes to avoid growing the list forever --- .../flywheel/backend/engine/DrawManager.java | 9 +++++++-- .../flywheel/backend/engine/MeshPool.java | 12 +++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java index b81502d1f..10562ee7f 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java @@ -58,8 +58,13 @@ public Plan createFramePlan() { public void flush(LightStorage lightStorage, EnvironmentStorage environmentStorage) { // Thread safety: flush is called from the render thread after all visual updates have been made, // so there are no:tm: threads we could be racing with. - for (var instancer : initializationQueue) { - initialize(instancer.key(), instancer.instancer()); + for (var init : initializationQueue) { + var instancer = init.instancer(); + if (instancer.instanceCount() > 0) { + initialize(init.key(), instancer); + } else { + instancers.remove(init.key()); + } } initializationQueue.clear(); } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/MeshPool.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/MeshPool.java index 5a41c1068..6cfa31424 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/MeshPool.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/MeshPool.java @@ -70,23 +70,17 @@ public void flush() { if (anyToRemove) { anyToRemove = false; processDeletions(); + } - // Might want to shrink the index pool if something was removed. - indexPool.reset(); - for (PooledMesh mesh : meshList) { - indexPool.updateCount(mesh.mesh.indexSequence(), mesh.indexCount()); - } - } else { + if (!recentlyAllocated.isEmpty()) { // Otherwise, just update the index with the new counts. for (PooledMesh mesh : recentlyAllocated) { indexPool.updateCount(mesh.mesh.indexSequence(), mesh.indexCount()); } + indexPool.flush(); recentlyAllocated.clear(); } - // Always need to flush the index pool. - indexPool.flush(); - uploadAll(); dirty = false; } From bedb92c73c9bea114d8dea44db3ddcbf7677b065 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sun, 10 Nov 2024 11:59:05 -0800 Subject: [PATCH 15/18] Lessen updates today! - LightLut now does incremental updates java-side - Still requires a full upload when changed, though it does not take up much space - ShaderLightVisualStorage now actually triggers removal of light section from the lut --- .../flywheel/backend/engine/LightLut.java | 83 ++++++++++++++----- .../flywheel/backend/engine/LightStorage.java | 28 ++++--- .../storage/ShaderLightVisualStorage.java | 6 +- 3 files changed, 83 insertions(+), 34 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java index d17ed806d..300d47da4 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java @@ -3,17 +3,16 @@ import java.util.function.BiConsumer; import java.util.function.Supplier; +import org.jetbrains.annotations.Nullable; + import it.unimi.dsi.fastutil.ints.IntArrayList; -import it.unimi.dsi.fastutil.longs.Long2IntMap; import net.minecraft.core.SectionPos; +// Massive kudos to RogueLogix for figuring out this LUT scheme. public final class LightLut { private final Layer> indices = new Layer<>(); - private LightLut() { - } - - private void add(long position, int index) { + public void add(long position, int index) { final var x = SectionPos.x(position); final var y = SectionPos.y(position); final var z = SectionPos.z(position); @@ -23,29 +22,30 @@ private void add(long position, int index) { .set(z, index + 1); } - private IntArrayList toLut() { - final var out = new IntArrayList(); - indices.fillLut(out, (yIndices, lut) -> yIndices.fillLut(lut, IntLayer::fillLut)); - return out; - } + public void remove(long section) { + final var x = SectionPos.x(section); + final var y = SectionPos.y(section); + final var z = SectionPos.z(section); + + var first = indices.get(x); - // Massive kudos to RogueLogix for figuring out this LUT scheme. - // TODO: switch to y x z or x z y ordering - // DATA LAYOUT - // [0] : base chunk X, X index count, followed by linear indices of y blocks - // [yBlockIndex] : baseChunk Y, Y index count, followed by linear indices of z blocks for this x - // [zBlockIndex] : baseChunk Z, Z index count, followed by linear indices of lighting chunks - // this data layout allows a single buffer to represent the lighting volume, without requiring the entire 3d lookup volume to be allocated - public static IntArrayList buildLut(Long2IntMap sectionIndicesMaps) { - if (sectionIndicesMaps.isEmpty()) { - return new IntArrayList(); + if (first == null) { + return; } - var out = new LightLut(); + var second = first.get(y); + + if (second == null) { + return; + } - sectionIndicesMaps.forEach(out::add); + second.clear(z); + } - return out.toLut(); + public IntArrayList flatten() { + final var out = new IntArrayList(); + indices.fillLut(out, (yIndices, lut) -> yIndices.fillLut(lut, IntLayer::fillLut)); + return out; } private static final class Layer { @@ -78,6 +78,25 @@ public void fillLut(IntArrayList lut, BiConsumer inner) { } } + @Nullable + public T get(int i) { + if (!hasBase) { + return null; + } + + if (i < base) { + return null; + } + + final var offset = i - base; + + if (offset >= nextLayer.length) { + return null; + } + + return (T) nextLayer[offset]; + } + public T computeIfAbsent(int i, Supplier ifAbsent) { if (!hasBase) { // We don't want to default to base 0, so we'll use the first value we get. @@ -155,6 +174,24 @@ public void set(int i, int index) { indices[offset] = index; } + public void clear(int i) { + if (!hasBase) { + return; + } + + if (i < base) { + return; + } + + final var offset = i - base; + + if (offset >= indices.length) { + return; + } + + indices[offset] = 0; + } + private void resize(int length) { final var newIndices = new int[length]; System.arraycopy(indices, 0, newIndices, 0, indices.length); diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java index 5b5ac1d51..f8b91be89 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightStorage.java @@ -44,12 +44,9 @@ public class LightStorage { private static final int INVALID_SECTION = -1; private final LevelAccessor level; - + private final LightLut lut; private final CpuArena arena; - private final Long2IntMap section2ArenaIndex = new Long2IntOpenHashMap(); - { - section2ArenaIndex.defaultReturnValue(INVALID_SECTION); - } + private final Long2IntMap section2ArenaIndex; private final BitSet changed = new BitSet(); private boolean needsLutRebuild = false; @@ -60,8 +57,10 @@ public class LightStorage { public LightStorage(LevelAccessor level) { this.level = level; - + lut = new LightLut(); arena = new CpuArena(SECTION_SIZE_BYTES, DEFAULT_ARENA_CAPACITY_SECTIONS); + section2ArenaIndex = new Long2IntOpenHashMap(); + section2ArenaIndex.defaultReturnValue(INVALID_SECTION); } /** @@ -135,12 +134,22 @@ private void removeUnusedSections() { if (!requestedSections.contains(section)) { arena.free(entry.getIntValue()); - needsLutRebuild = true; + endTrackingSection(section); it.remove(); } } } + private void beginTrackingSection(long section, int index) { + lut.add(section, index); + needsLutRebuild = true; + } + + private void endTrackingSection(long section) { + lut.remove(section); + needsLutRebuild = true; + } + public int capacity() { return arena.capacity(); } @@ -374,7 +383,7 @@ private int indexForSection(long section) { if (out == INVALID_SECTION) { out = arena.alloc(); section2ArenaIndex.put(section, out); - needsLutRebuild = true; + beginTrackingSection(section, out); } return out; } @@ -406,8 +415,7 @@ public void upload(GlBuffer buffer) { } public IntArrayList createLut() { - // TODO: incremental lut updates - return LightLut.buildLut(section2ArenaIndex); + return lut.flatten(); } private enum SectionEdge { diff --git a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/storage/ShaderLightVisualStorage.java b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/storage/ShaderLightVisualStorage.java index 052a9e06c..804f98bae 100644 --- a/common/src/main/java/dev/engine_room/flywheel/impl/visualization/storage/ShaderLightVisualStorage.java +++ b/common/src/main/java/dev/engine_room/flywheel/impl/visualization/storage/ShaderLightVisualStorage.java @@ -43,7 +43,11 @@ public void add(ShaderLightVisual visual, SectionTracker tracker) { } public void remove(ShaderLightVisual visual) { - trackers.remove(visual); + var tracker = trackers.remove(visual); + + if (tracker != null) { + markDirty(); + } } public void clear() { From 671d47a1367dfc86caa0ab017dc16176bf6f72c6 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Sun, 10 Nov 2024 12:10:47 -0800 Subject: [PATCH 16/18] Swizzle - Switch to Y X Z ordering - In theory this will be more coherent since the first lut step on the GPU will have a more constrained range of values in the worst case --- .../engine_room/flywheel/backend/engine/LightLut.java | 9 +++++---- .../assets/flywheel/flywheel/internal/light_lut.glsl | 10 +++++----- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java index 300d47da4..885c616dd 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/LightLut.java @@ -9,6 +9,7 @@ import net.minecraft.core.SectionPos; // Massive kudos to RogueLogix for figuring out this LUT scheme. +// First layer is Y, then X, then Z. public final class LightLut { private final Layer> indices = new Layer<>(); @@ -17,8 +18,8 @@ public void add(long position, int index) { final var y = SectionPos.y(position); final var z = SectionPos.z(position); - indices.computeIfAbsent(x, Layer::new) - .computeIfAbsent(y, IntLayer::new) + indices.computeIfAbsent(y, Layer::new) + .computeIfAbsent(x, IntLayer::new) .set(z, index + 1); } @@ -27,13 +28,13 @@ public void remove(long section) { final var y = SectionPos.y(section); final var z = SectionPos.z(section); - var first = indices.get(x); + var first = indices.get(y); if (first == null) { return; } - var second = first.get(y); + var second = first.get(x); if (second == null) { return; diff --git a/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl b/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl index 355d2290f..7a939857b 100644 --- a/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl +++ b/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl @@ -43,18 +43,18 @@ bool _flw_nextLut(uint base, int coord, out uint next) { } bool _flw_chunkCoordToSectionIndex(ivec3 sectionPos, out uint index) { - uint y; - if (_flw_nextLut(0, sectionPos.x, y) || y == 0) { + uint first; + if (_flw_nextLut(0, sectionPos.y, first) || first == 0) { return true; } - uint z; - if (_flw_nextLut(y, sectionPos.y, z) || z == 0) { + uint second; + if (_flw_nextLut(first, sectionPos.x, second) || second == 0) { return true; } uint sectionIndex; - if (_flw_nextLut(z, sectionPos.z, sectionIndex) || sectionIndex == 0) { + if (_flw_nextLut(second, sectionPos.z, sectionIndex) || sectionIndex == 0) { return true; } From 67096827857e3de0db0e7820b4ce20c1d1ffb460 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Mon, 11 Nov 2024 18:25:57 -0800 Subject: [PATCH 17/18] A little less lit - Fix normalizing constant in light_lut.glsl to make shader light slightly darker and consistent with chunk lighting --- .../assets/flywheel/flywheel/internal/light_lut.glsl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl b/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl index 7a939857b..e9ec12039 100644 --- a/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl +++ b/common/src/backend/resources/assets/flywheel/flywheel/internal/light_lut.glsl @@ -14,6 +14,8 @@ const float _FLW_EPSILON = 1e-5; const uint _FLW_LOWER_10_BITS = 0x3FFu; const uint _FLW_UPPER_10_BITS = 0xFFF00000u; +const float _FLW_LIGHT_NORMALIZER = 1. / 16.; + uint _flw_indexLut(uint index); uint _flw_indexLight(uint index); @@ -98,7 +100,7 @@ bool flw_lightFetch(ivec3 blockPos, out vec2 lightCoord) { uvec3 blockInSectionPos = (blockPos & 0xF) + 1; - lightCoord = vec2(_flw_lightAt(sectionOffset, blockInSectionPos)) / 15.; + lightCoord = vec2(_flw_lightAt(sectionOffset, blockInSectionPos)) * _FLW_LIGHT_NORMALIZER; return true; } @@ -298,7 +300,7 @@ vec3 _flw_lightForDirection(uint[27] lights, vec3 interpolant, uint c00, uint c0 vec3 light = mix(light0, light1, interpolant.y); // Normalize the light coords - light.xy *= 1. / 15.; + light.xy *= _FLW_LIGHT_NORMALIZER; // Calculate the AO multiplier from the number of valid blocks light.z = _flw_validCountToAo(light.z); @@ -351,7 +353,7 @@ bool flw_light(vec3 worldPos, vec3 normal, out FlwLightAo light) { vec2 light0 = mix(light00, light01, interpolant.y); vec2 light1 = mix(light10, light11, interpolant.y); - light.light = mix(light0, light1, interpolant.x) / 15.; + light.light = mix(light0, light1, interpolant.x) * _FLW_LIGHT_NORMALIZER; light.ao = 1.; // Lighting and AO accurate to chunk baking @@ -410,7 +412,7 @@ bool flw_light(vec3 worldPos, vec3 normal, out FlwLightAo light) { // Entirely flat lighting, the lowest setting and a fallback in case an invalid option is set #else - light.light = vec2(_flw_lightAt(sectionOffset, blockInSectionPos)) / 15.; + light.light = vec2(_flw_lightAt(sectionOffset, blockInSectionPos)) * _FLW_LIGHT_NORMALIZER; light.ao = 1.; #endif From 868a263c287a5266f64f971eda4272ed5ed6e0b7 Mon Sep 17 00:00:00 2001 From: Jozufozu Date: Mon, 11 Nov 2024 22:33:41 -0800 Subject: [PATCH 18/18] No one can hear you free - Silence empty model warning behind a system property - Fix gpu memory leak from light/matrix buffers on indirect --- .../flywheel/backend/engine/DrawManager.java | 17 ++++++++++------- .../engine/indirect/IndirectDrawManager.java | 4 ++++ .../backend/engine/indirect/LightBuffers.java | 5 +++++ .../backend/engine/indirect/MatrixBuffer.java | 4 ++++ 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java index 10562ee7f..9f5c4e2ed 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/DrawManager.java @@ -28,6 +28,8 @@ import net.minecraft.client.resources.model.ModelBakery; public abstract class DrawManager> { + private static final boolean WARN_EMPTY_MODELS = Boolean.getBoolean("flywheel.warnEmptyModels"); + /** * A map of instancer keys to instancers. *
@@ -100,15 +102,16 @@ private static boolean checkAndWarnEmptyModel(Model model) { return true; } - StringBuilder builder = new StringBuilder(); - builder.append("Creating an instancer for a model with no meshes! Stack trace:"); + if (WARN_EMPTY_MODELS) { + StringBuilder builder = new StringBuilder(); + builder.append("Creating an instancer for a model with no meshes! Stack trace:"); - StackWalker.getInstance() - // .walk(s -> s.skip(3)) // this causes forEach to crash for some reason - .forEach(f -> builder.append("\n\t") - .append(f.toString())); + StackWalker.getInstance() + .forEach(f -> builder.append("\n\t") + .append(f.toString())); - FlwBackend.LOGGER.warn(builder.toString()); + FlwBackend.LOGGER.warn(builder.toString()); + } return false; } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java index 8a6dd5e4b..65c0d4aba 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/IndirectDrawManager.java @@ -186,6 +186,10 @@ public void delete() { programs.release(); depthPyramid.delete(); + + lightBuffers.delete(); + + matrixBuffer.delete(); } public void renderCrumbling(List crumblingBlocks) { diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/LightBuffers.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/LightBuffers.java index 9db63afb0..7f78c1e46 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/LightBuffers.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/LightBuffers.java @@ -40,4 +40,9 @@ public void bind() { GL46.glBindBufferRange(GL46.GL_SHADER_STORAGE_BUFFER, BufferBindings.LIGHT_LUT, lut.handle(), 0, lut.byteCapacity()); GL46.glBindBufferRange(GL46.GL_SHADER_STORAGE_BUFFER, BufferBindings.LIGHT_SECTION, sections.handle(), 0, sections.byteCapacity()); } + + public void delete() { + lut.delete(); + sections.delete(); + } } diff --git a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/MatrixBuffer.java b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/MatrixBuffer.java index ce579c465..7328bcb3b 100644 --- a/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/MatrixBuffer.java +++ b/common/src/backend/java/dev/engine_room/flywheel/backend/engine/indirect/MatrixBuffer.java @@ -30,4 +30,8 @@ public void bind() { GL46.glBindBufferRange(GL46.GL_SHADER_STORAGE_BUFFER, BufferBindings.MATRICES, matrices.handle(), 0, matrices.byteCapacity()); } + + public void delete() { + matrices.delete(); + } }