-
-
Notifications
You must be signed in to change notification settings - Fork 611
Membrane stretch #7084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Membrane stretch #7084
Changes from all commits
8cdeb63
a0c5c5c
2936878
6e9edc5
40a3837
4d00df7
583071b
fa6767b
25057c1
2974e2f
a01ba9f
04e62bd
1f40548
53d9466
6aa589e
0cfb7ae
01c7623
18b75de
d79064a
beaba08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,25 +10,57 @@ public interface IMembraneDataSource | |
| { | ||
| public Vector2[] HexPositions { get; } | ||
| public int HexPositionCount { get; } | ||
| public Vector2[]? MulticellularPositions { get; } | ||
| public Vector2? CellPositionInMulticellular { get; } | ||
| public int[]? MulticellularOrientations { get; } | ||
| public int? CellOrientation { get; } | ||
| public long? ColonyKey { get; } | ||
|
Comment on lines
+16
to
+17
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two are boxed, so I think these should use a bool flag and a direct primitive type. Also just noticed that CellPositionInMulticellular is also boxed. Also this class still has a ton of lists, I'd prefer a single struct that combined public struct MembraneGenerationCellData{
public Vector2 Position;
public int CellOrientation;
}And then having one list with multicellular data using that struct. |
||
| public MembraneType Type { get; } | ||
| public bool IsPreMulticellularStretch { get; } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Struct that holds parameters about a membrane generation request | ||
| /// </summary> | ||
| public struct MembraneGenerationParameters : IMembraneDataSource | ||
| { | ||
| public MembraneGenerationParameters(Vector2[] hexPositions, int hexPositionCount, MembraneType type, | ||
| Vector2[] multicellularPositions, Vector2 thisCellPosition, int[]? multicellularOrientations, | ||
| int? thisCellOrientation, long? colonyKey = null, bool isPreMulticellularStretch = false) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nullable primitives also cause boxing here, so should be avoided. Maybe with constructor overloads where the optional ones are just missing? |
||
| : this(hexPositions, hexPositionCount, type) | ||
| { | ||
| MulticellularPositions = multicellularPositions; | ||
| CellPositionInMulticellular = thisCellPosition; | ||
| MulticellularOrientations = multicellularOrientations; | ||
| CellOrientation = thisCellOrientation; | ||
| ColonyKey = colonyKey; | ||
| IsPreMulticellularStretch = isPreMulticellularStretch; | ||
| } | ||
|
|
||
| public MembraneGenerationParameters(Vector2[] hexPositions, int hexPositionCount, MembraneType type) | ||
| { | ||
| HexPositions = hexPositions; | ||
| HexPositionCount = hexPositionCount; | ||
| Type = type; | ||
| } | ||
|
|
||
| public int[]? MulticellularOrientations { get; } | ||
|
|
||
| public int? CellOrientation { get; } | ||
|
|
||
| public long? ColonyKey { get; } | ||
|
|
||
| public Vector2[] HexPositions { get; } | ||
|
|
||
| public Vector2[]? MulticellularPositions { get; } | ||
|
|
||
| public Vector2? CellPositionInMulticellular { get; } | ||
|
|
||
| public int HexPositionCount { get; } | ||
|
|
||
| public MembraneType Type { get; } | ||
|
|
||
| public bool IsPreMulticellularStretch { get; set; } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -89,7 +121,7 @@ public static MembranePointData GetOrComputeMembraneShape(IReadOnlyList<IPositio | |
|
|
||
| var cache = ProceduralDataCache.Instance; | ||
|
|
||
| var hash = ComputeMembraneDataHash(hexes, length, membraneType); | ||
| var hash = new MembraneGenerationParameters(hexes, length, membraneType).ComputeMembraneDataHash(); | ||
|
|
||
| var result = cache.ReadMembraneData(hash); | ||
|
|
||
|
|
@@ -105,63 +137,151 @@ public static MembranePointData GetOrComputeMembraneShape(IReadOnlyList<IPositio | |
| // Need to compute the data now, it doesn't exist in the cache | ||
| var generator = MembraneShapeGenerator.GetThreadSpecificGenerator(); | ||
|
|
||
| result = generator.GenerateShape(hexes, length, membraneType); | ||
| result = generator.GenerateMicrobeShape(hexes, length, membraneType); | ||
|
|
||
| cache.WriteMembraneData(ref result); | ||
| return result; | ||
| } | ||
|
|
||
| public static long ComputeMembraneDataHash(Vector2[] positions, int count, MembraneType type) | ||
| public static long ComputeMembraneDataHash(this IMembraneDataSource dataSource) | ||
| { | ||
| var nameHash = type.InternalName.GetHashCode(); | ||
| const long prime1 = 1099511628211L; | ||
| const long prime2 = 1409; | ||
| const long prime3 = 7793; | ||
|
|
||
| var nameHash = dataSource.Type.InternalName.GetHashCode(); | ||
|
|
||
| unchecked | ||
| { | ||
| long hash = 1409 + nameHash + ((long)nameHash << 28); | ||
| long hash = prime2 + nameHash + ((long)nameHash << 28); | ||
|
|
||
| hash ^= (dataSource.HexPositionCount + 1) * prime3; | ||
|
|
||
| for (int i = 0; i < dataSource.HexPositionCount; ++i) | ||
| { | ||
| hash = (hash * prime1) ^ BitConverter.SingleToInt32Bits(dataSource.HexPositions[i].X); | ||
| hash = (hash * prime1) ^ BitConverter.SingleToInt32Bits(dataSource.HexPositions[i].Y); | ||
|
Comment on lines
+162
to
+163
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be more efficient to read |
||
| } | ||
|
|
||
| hash ^= (count + 1) * 7793; | ||
| int hashMultiply = 1; | ||
| // NOTE: ColonyKey alone is the same for every cell in a colony, so it cannot be the only thing | ||
| // distinguishing membrane data between cells that share a hex layout. The per-cell position/orientation | ||
| // must also be part of the hash. | ||
| if (dataSource.CellPositionInMulticellular != null) | ||
| { | ||
| hash = (hash * prime1) ^ | ||
| BitConverter.SingleToInt32Bits(dataSource.CellPositionInMulticellular.Value.X); | ||
| hash = (hash * prime1) ^ | ||
| BitConverter.SingleToInt32Bits(dataSource.CellPositionInMulticellular.Value.Y); | ||
| } | ||
|
|
||
| if (dataSource.CellOrientation != null) | ||
| { | ||
| hash = (hash * prime1) ^ dataSource.CellOrientation.Value; | ||
| } | ||
|
|
||
| for (int i = 0; i < count; ++i) | ||
| if (dataSource.ColonyKey != null) | ||
| { | ||
| var posHash = positions[i].GetHashCode(); | ||
| hash = (hash * prime1) ^ dataSource.ColonyKey.Value; | ||
| } | ||
|
|
||
| // TODO: switch to using rotate left here once we can (after Godot 4) | ||
| hash ^= (hashMultiply * posHash) ^ ((5081L * hashMultiply * hashMultiply + posHash) << 32); | ||
| ++hashMultiply; | ||
| if (dataSource.IsPreMulticellularStretch) | ||
| { | ||
| hash = (hash * prime1) ^ 1; | ||
| } | ||
|
|
||
| return hash; | ||
| } | ||
| } | ||
|
|
||
| public static long ComputeMembraneDataHash(this IMembraneDataSource dataSource) | ||
| { | ||
| return ComputeMembraneDataHash(dataSource.HexPositions, dataSource.HexPositionCount, dataSource.Type); | ||
| } | ||
|
|
||
| public static bool MembraneDataFieldsEqual(this IMembraneDataSource dataSource, IMembraneDataSource other) | ||
| { | ||
| return dataSource.MembraneDataFieldsEqual(other.HexPositions, other.HexPositionCount, other.Type); | ||
| return dataSource.MembraneDataFieldsEqual(other.HexPositions, other.HexPositionCount, other.Type, | ||
| other.CellPositionInMulticellular, other.CellOrientation, other.ColonyKey); | ||
| } | ||
|
|
||
| public static bool MembraneDataFieldsEqual(this IMembraneDataSource dataSource, Vector2[] otherPoints, | ||
| int otherPointCount, MembraneType otherType) | ||
| int otherPointCount, MembraneType otherType, Vector2? cellPositionInMulticellular = null, | ||
| int? cellOrientationInMulticellular = null, long? otherColonyKey = null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Boxing happening here as well. |
||
| { | ||
| if (!dataSource.Type.Equals(otherType)) | ||
| { | ||
| GD.PrintErr($"Membrane cache Type mismatch: {dataSource.Type} != {otherType}"); | ||
| return false; | ||
| } | ||
|
|
||
| if (dataSource.HexPositionCount != otherPointCount) | ||
| { | ||
| GD.PrintErr("Membrane cache HexPositionCount mismatch: " + | ||
| $"{dataSource.HexPositionCount} != {otherPointCount}"); | ||
| return false; | ||
| } | ||
|
|
||
| var count = dataSource.HexPositionCount; | ||
|
|
||
| var sourcePoints = dataSource.HexPositions; | ||
|
|
||
| // Per-cell position must match — this is what actually distinguishes cells within the same colony | ||
| if (dataSource.CellPositionInMulticellular != null || cellPositionInMulticellular != null) | ||
| { | ||
| if (dataSource.CellPositionInMulticellular == null || cellPositionInMulticellular == null) | ||
| { | ||
| GD.PrintErr("Membrane cache CellPositionInMulticellular null mismatch: " + | ||
| $"source={dataSource.CellPositionInMulticellular == null} " + | ||
| $"other={cellPositionInMulticellular == null}"); | ||
| return false; | ||
| } | ||
|
|
||
| if (!dataSource.CellPositionInMulticellular.Value.Equals(cellPositionInMulticellular.Value)) | ||
| { | ||
| GD.PrintErr("Membrane cache CellPositionInMulticellular mismatch: " + | ||
| $"{dataSource.CellPositionInMulticellular} != {cellPositionInMulticellular}"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if (dataSource.CellOrientation != null || cellOrientationInMulticellular != null) | ||
| { | ||
| if (dataSource.CellOrientation == null || cellOrientationInMulticellular == null) | ||
| { | ||
| GD.PrintErr("Membrane cache CellOrientation null mismatch: " + | ||
| $"source={dataSource.CellOrientation == null} " + | ||
| $"other={cellOrientationInMulticellular == null}"); | ||
| return false; | ||
| } | ||
|
|
||
| if (dataSource.CellOrientation.Value != cellOrientationInMulticellular.Value) | ||
| { | ||
| GD.PrintErr("Membrane cache CellOrientation mismatch: " + | ||
| $"{dataSource.CellOrientation} != {cellOrientationInMulticellular}"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if (dataSource.ColonyKey != null || otherColonyKey != null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at things here, I would make it so that in release mode only the colony key is checked and the above orientation and position checks are inside a |
||
| { | ||
| if (dataSource.ColonyKey == null || otherColonyKey == null) | ||
| { | ||
| GD.PrintErr("Membrane cache ColonyKey null mismatch: " + | ||
| $"source={dataSource.ColonyKey == null} other={otherColonyKey == null}"); | ||
| return false; | ||
| } | ||
|
|
||
| if (dataSource.ColonyKey.Value != otherColonyKey.Value) | ||
| { | ||
| GD.PrintErr( | ||
| $"Membrane cache ColonyKey mismatch: {dataSource.ColonyKey.Value} != {otherColonyKey.Value}"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| for (int i = 0; i < count; ++i) | ||
| { | ||
| if (sourcePoints[i] != otherPoints[i]) | ||
| { | ||
| GD.PrintErr($"Membrane cache HexPositions[{i}] mismatch: " + | ||
| $"{sourcePoints[i]} != {otherPoints[i]}"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding all of this extra data makes this a ton heavier, enough so that I fear for the efficiency of the overall design being able to handle things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you mean by "being able to handle things"? I unfortunatelly need all of this data to make good membrane stretch generation. What would you propose here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I made the hashing and finding stuff is that I bedgrudginly accepted that the game has to hash the full list of positions, and that works fine now. But this PR increases the amount of hashing effort 5x, and makes membranes unique based on colony position, meaning that the overall caching system for membranes might not even make sense. So by adding a lot of heavy weight to the membrane caching system might hinder it so much that the entire concept needs to be rethought.
I suggested a few things to try to make it lighter (by using a single list, and only verifying some properties), but there might be a fundamental limit that causes problems with doing so much new stuff in the membrane caching and generation.