Skip to content
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.

Sign up for GitHub

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

Spark performance improvements #4459

Merged
merged 11 commits into from
Oct 15, 2023
Merged

Spark performance improvements #4459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
19b61b7
Initial spark perf improvements
anonymous123-code May 31, 2023
c0478d9
Cache getManaReceiver
anonymous123-code Jun 1, 2023
8cceb0e
Fix some transfer update bugs
anonymous123-code Jun 1, 2023
d3c0aa1
Integration with other blocks fixes
anonymous123-code Jun 2, 2023
1920c22
Final changes: Remove unnecessary removeTransfers
anonymous123-code Jun 2, 2023
a81f652
Fix weird behavior when transferring low mana amounts
anonymous123-code Jun 3, 2023
443e222
Better Caching impl
anonymous123-code Jun 3, 2023
0c1fe1e
Rebase fixes and remove caching
anonymous123-code Oct 7, 2023
a6a21fd
Comment, rename wasFull for better readability
anonymous123-code Oct 8, 2023
5eb322a
Format
anonymous123-code Oct 8, 2023
d33ba5d
Address review:
anonymous123-code Oct 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Xplat/src/main/java/vazkii/botania/api/mana/spark/ManaSpark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,19 @@ public interface ManaSpark extends SparkEntity {
/**
* Gets a collection of all Sparks this is tranfering to.
*/
Collection<ManaSpark> getTransfers();
Collection<ManaSpark> getOutgoingTransfers();

/**
* Registers the Spark passed in as a Spark meant for mana to be transfered towards.
*/
void registerTransfer(ManaSpark entity);

/**
* Makes that spark register transfers for all relevant sparks, needs to be called whenever the connected sparks may
* change (setNetwork, remove and setUpgrade should already do this)
*/
void updateTransfers();

SparkUpgradeType getUpgrade();

void setUpgrade(SparkUpgradeType upgrade);
Expand Down
5 changes: 0 additions & 5 deletions Xplat/src/main/java/vazkii/botania/common/block/block_entity/SparkTinkererBlockEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import vazkii.botania.xplat.XplatAbstractions;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;

public class SparkTinkererBlockEntity extends ExposedSimpleInventoryBlockEntity {
Expand Down Expand Up @@ -59,10 +58,6 @@ public void doSwap() {
ItemStack sparkStack = SparkAugmentItem.getByType(upg);
SparkUpgradeType newUpg = changeStack.isEmpty() ? SparkUpgradeType.NONE : ((SparkAugmentItem) changeStack.getItem()).type;
spark.setUpgrade(newUpg);
Collection<ManaSpark> transfers = spark.getTransfers();
if (transfers != null) {
transfers.clear();
}
getItemHandler().setItem(0, sparkStack);
}
}
Expand Down
7 changes: 2 additions & 5 deletions .../src/main/java/vazkii/botania/common/block/block_entity/mana/ManaSpreaderBlockEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ public static void commonTick(Level level, BlockPos worldPosition, BlockState st
for (Direction dir : Direction.values()) {
var relPos = worldPosition.relative(dir);
if (level.hasChunkAt(relPos)) {
var receiverAt = XplatAbstractions.INSTANCE.findManaReceiver(level, relPos,
level.getBlockState(relPos), level.getBlockEntity(relPos), dir.getOpposite());
var receiverAt = XplatAbstractions.INSTANCE.findManaReceiver(level, relPos, dir.getOpposite());
if (receiverAt instanceof ManaPool pool) {
if (wasInNetwork && (pool != self.receiver || self.getVariant() == ManaSpreaderBlock.Variant.REDSTONE)) {
if (pool instanceof KeyLocked locked && !locked.getOutputKey().equals(self.getInputKey())) {
Expand Down Expand Up @@ -348,9 +347,7 @@ public void readPacketNBT(CompoundTag cmp) {
int z = cmp.getInt(TAG_FORCE_CLIENT_BINDING_Z);
if (y != Integer.MIN_VALUE) {
var pos = new BlockPos(x, y, z);
var state = level.getBlockState(pos);
var be = level.getBlockEntity(pos);
receiver = XplatAbstractions.INSTANCE.findManaReceiver(level, pos, state, be, null);
receiver = XplatAbstractions.INSTANCE.findManaReceiver(level, pos, null);
} else {
receiver = null;
}
Expand Down
130 changes: 97 additions & 33 deletions Xplat/src/main/java/vazkii/botania/common/entity/ManaSparkEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,13 @@ public class ManaSparkEntity extends SparkBaseEntity implements ManaSpark {
private static final String TAG_UPGRADE = "upgrade";
private static final EntityDataAccessor<Integer> UPGRADE = SynchedEntityData.defineId(ManaSparkEntity.class, EntityDataSerializers.INT);

private final Set<ManaSpark> transfers = Collections.newSetFromMap(new WeakHashMap<>());
private final Set<ManaSpark> outgoingTransfers = Collections.newSetFromMap(new WeakHashMap<>());

private int removeTransferants = 2;
private final ArrayList<ManaSpark> transfersTowardsSelfToRegister = new ArrayList<>();
anonymous123-code marked this conversation as resolved.
Show resolved Hide resolved

private boolean shouldFilterTransfers = true;
private boolean receiverWasFull = true;
private boolean firstTick = true;

public ManaSparkEntity(EntityType<ManaSparkEntity> type, Level world) {
super(type, world);
Expand Down Expand Up @@ -89,6 +93,11 @@ public void tick() {
return;
}

// When loaded, initialize transfers
if (firstTick) {
updateTransfers();
}

SparkAttachable tile = getAttachedTile();
if (tile == null) {
dropAndKill();
Expand All @@ -97,7 +106,7 @@ public void tick() {
var receiver = getAttachedManaReceiver();

SparkUpgradeType upgrade = getUpgrade();
Collection<ManaSpark> transfers = getTransfers();
Collection<ManaSpark> transfers = getOutgoingTransfers();

switch (upgrade) {
case DISPERSIVE -> {
Expand Down Expand Up @@ -163,29 +172,25 @@ public void tick() {

}
case DOMINANT -> {
List<ManaSpark> validSparks = SparkHelper.getSparksAround(level(), getX(), getY() + (getBbHeight() / 2), getZ(), getNetwork());
validSparks.removeIf(s -> {
SparkUpgradeType otherUpgrade = s.getUpgrade();
return s == this || otherUpgrade != SparkUpgradeType.NONE || !(s.getAttachedManaReceiver() instanceof ManaPool);
});
if (!validSparks.isEmpty()) {
validSparks.get(level().random.nextInt(validSparks.size())).registerTransfer(this);
if (receiverWasFull && !receiver.isFull()) {
updateTransfers();
}
if (!transfersTowardsSelfToRegister.isEmpty()) {
transfersTowardsSelfToRegister.remove(transfersTowardsSelfToRegister.size() - 1).registerTransfer(this);
}

}
case RECESSIVE -> {
var otherSparks = SparkHelper.getSparksAround(level(), getX(), getY() + (getBbHeight() / 2), getZ(), getNetwork());
for (var otherSpark : otherSparks) {
SparkUpgradeType otherUpgrade = otherSpark.getUpgrade();
if (otherSpark != this
&& otherUpgrade != SparkUpgradeType.DOMINANT
&& otherUpgrade != SparkUpgradeType.RECESSIVE
&& otherUpgrade != SparkUpgradeType.ISOLATED) {
transfers.add(otherSpark);
}
// Recessive does not need to be handled because recessive sparks get notified in all relevant cases
default -> {
if (receiverWasFull && !receiver.isFull()) {
notifyOthers(getNetwork());
}
}
default -> {}
}

if (receiver != null) {
receiverWasFull = receiver.isFull();
} else {
receiverWasFull = true;
}

if (!transfers.isEmpty()) {
Expand All @@ -194,11 +199,17 @@ public void tick() {
int manaSpent = 0;

if (manaTotal > 0) {
if (shouldFilterTransfers) {
filterTransfers();
shouldFilterTransfers = false;
}

for (ManaSpark spark : transfers) {
count--;
SparkAttachable attached = spark.getAttachedTile();
var attachedReceiver = spark.getAttachedManaReceiver();
if (attached == null || attachedReceiver == null || attachedReceiver.isFull() || spark.areIncomingTransfersDone()) {
shouldFilterTransfers = true;
continue;
}

Expand All @@ -212,8 +223,36 @@ public void tick() {
}
}

if (removeTransferants > 0) {
removeTransferants--;
firstTick = false;
}

@Override
public void updateTransfers() {
transfersTowardsSelfToRegister.clear();
switch (getUpgrade()) {
case RECESSIVE -> {
var otherSparks = SparkHelper.getSparksAround(level(), getX(), getY() + (getBbHeight() / 2), getZ(), getNetwork());
Collections.shuffle(otherSparks);
for (var otherSpark : otherSparks) {
SparkUpgradeType otherUpgrade = otherSpark.getUpgrade();
if (otherSpark != this
&& otherUpgrade != SparkUpgradeType.DOMINANT
&& otherUpgrade != SparkUpgradeType.RECESSIVE
&& otherUpgrade != SparkUpgradeType.ISOLATED) {
outgoingTransfers.add(otherSpark);
}
}
}
case DOMINANT -> {
List<ManaSpark> validSparks = SparkHelper.getSparksAround(level(), getX(), getY() + (getBbHeight() / 2), getZ(), getNetwork());
for (var spark : validSparks) {
SparkUpgradeType otherUpgrade = spark.getUpgrade();
if (spark != this && otherUpgrade == SparkUpgradeType.NONE && spark.getAttachedManaReceiver() instanceof ManaPool) {
transfersTowardsSelfToRegister.add(spark);
}
}
Collections.shuffle(transfersTowardsSelfToRegister);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reasoning for the shuffles that were added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise mana pools would be loaded in a relatively consistent order, making only some fill up. This makes it so if they're reloaded, that order is shuffled at least sometimes. A similar thing was in here before, I'm only guessing the reason why it has been implemented: https://github.com/VazkiiMods/Botania/pull/4459/files#diff-6ab7cbbb33c3d206fc8ca670958f924a1353804a7fb8f234f3308622d23723f7L172
I can also add the transfers directly, but that would be slightly breaking with older behavior (Previosly, and in this implementation dominant sparks would only add one transfer each tick, taking some time to initialize). This would make transfersTowardsSelfToRegister unnecessary.

}
}
filterTransfers();
}
Expand Down Expand Up @@ -244,6 +283,12 @@ private void dropAndKill() {
discard();
}

@Override
public void remove(RemovalReason removalReason) {
super.remove(removalReason);
notifyOthers(getNetwork());
}

@Override
public InteractionResult interact(Player player, InteractionHand hand) {
ItemStack stack = player.getItemInHand(hand);
Expand All @@ -256,8 +301,9 @@ public InteractionResult interact(Player player, InteractionHand hand) {
spawnAtLocation(SparkAugmentItem.getByType(upgrade), 0F);
setUpgrade(SparkUpgradeType.NONE);

transfers.clear();
removeTransferants = 2;
// Recalculate transfers, recessive and dominant will register the proper transfers
outgoingTransfers.clear();
notifyOthers(getNetwork());
} else {
dropAndKill();
}
Expand Down Expand Up @@ -318,7 +364,7 @@ public ManaReceiver getAttachedManaReceiver() {
}

private void filterTransfers() {
Iterator<ManaSpark> iter = transfers.iterator();
Iterator<ManaSpark> iter = outgoingTransfers.iterator();
while (iter.hasNext()) {
ManaSpark spark = iter.next();
SparkUpgradeType upgr = getUpgrade();
Expand All @@ -340,21 +386,27 @@ private void filterTransfers() {
}

@Override
public Collection<ManaSpark> getTransfers() {
filterTransfers();
return transfers;
public Collection<ManaSpark> getOutgoingTransfers() {
return outgoingTransfers;
}

private boolean hasTransfer(ManaSpark entity) {
return transfers.contains(entity);
return outgoingTransfers.contains(entity);
}

@Override
public void registerTransfer(ManaSpark entity) {
if (hasTransfer(entity)) {
return;
}
transfers.add(entity);
outgoingTransfers.add(entity);
filterTransfers();
}

private void notifyOthers(DyeColor network) {
for (var spark : SparkHelper.getSparksAround(level(), getX(), getY() + (getBbHeight() / 2), getZ(), network)) {
spark.updateTransfers();
}
}

@Override
Expand All @@ -365,12 +417,24 @@ public SparkUpgradeType getUpgrade() {
@Override
public void setUpgrade(SparkUpgradeType upgrade) {
entityData.set(UPGRADE, upgrade.ordinal());
updateTransfers();
notifyOthers(getNetwork());
}

@Override
public void setNetwork(DyeColor color) {
// The previous network needs to filter this spark out
var previousNetwork = getNetwork();
super.setNetwork(color);
updateTransfers();
notifyOthers(color);
notifyOthers(previousNetwork);
}

@Override
public boolean areIncomingTransfersDone() {
if (getAttachedManaReceiver() instanceof ManaPool) {
return removeTransferants > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I'm not really sure what this old code did lol, do you know and can you explain why it should be removed?

Copy link
Contributor Author

@anonymous123-code anonymous123-code Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RemoveTransferants was the old reset logic when stuff like the upgrade or the network changed. This is now handled differently. And the only reason why it might be removed is if it is full, which is checked seperately, so I changed it to be a constant false

return false;
}

SparkAttachable attachable = getAttachedTile();
Expand Down
4 changes: 1 addition & 3 deletions Xplat/src/main/java/vazkii/botania/common/item/FloralObedienceStickItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ public InteractionResult useOn(UseOnContext ctx) {
}

public static boolean applyStick(Level world, BlockPos pos) {
var state = world.getBlockState(pos);
var be = world.getBlockEntity(pos);
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(world, pos, state, be, null);
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(world, pos, null);
if (receiver instanceof ManaPool || receiver instanceof ManaCollector) {
int range = receiver instanceof ManaPool ? FunctionalFlowerBlockEntity.LINK_RANGE : GeneratingFlowerBlockEntity.LINK_RANGE;

Expand Down
4 changes: 1 addition & 3 deletions Xplat/src/main/java/vazkii/botania/common/item/lens/MagnetizingLens.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ public void updateBurst(ManaBurst burst, ItemStack stack) {
final boolean sourceless = source.equals(ManaBurst.NO_SOURCE);

Predicate<BlockPos> predicate = pos -> {
var state = entity.level().getBlockState(pos);
var be = entity.level().getBlockEntity(pos);
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(entity.level(), pos, state, be, null);
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(entity.level(), pos, null);
return receiver != null
&& (sourceless || pos.distSqr(source) > 9)
&& receiver.canReceiveManaFromBursts()
Expand Down
3 changes: 1 addition & 2 deletions Xplat/src/main/java/vazkii/botania/common/item/lens/RedirectiveLens.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ private void handleHitBlock(ManaBurst burst, BlockHitResult result) {

var entity = burst.entity();
var hitPos = result.getBlockPos();
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(entity.level(), hitPos,
entity.level().getBlockState(hitPos), entity.level().getBlockEntity(hitPos), result.getDirection());
var receiver = XplatAbstractions.INSTANCE.findManaReceiver(entity.level(), hitPos, result.getDirection());
if (receiver instanceof ManaSpreader spreader) {
Vec3 tileVec = Vec3.atCenterOf(hitPos);
Vec3 diffVec = sourceVec.subtract(tileVec);
Expand Down
Loading