Skip to content

Commit e54a683

Browse files
octylFractaloctylFractal
committed
Fix SideEffectSet inconsistencies, improve perf
The `apply` term was inconsistent in this API, but should have always been the same. This makes the "set" actually store the state of the given effect, even if it's the default, and therefore consistent.
1 parent cfd9eea commit e54a683

3 files changed

Lines changed: 187 additions & 21 deletions

File tree

‎worldedit-core/src/main/java/com/sk89q/worldedit/EditSession.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ public void setSideEffectApplier(SideEffectSet sideEffectSet) {
559559
*/
560560
@Deprecated
561561
public boolean hasFastMode() {
562-
return sideEffectExtent != null && this.sideEffectExtent.getSideEffectSet().doesApplyAny();
562+
return sideEffectExtent != null && !this.sideEffectExtent.getSideEffectSet().doesApplyAny();
563563
}
564564

565565
public SideEffectSet getSideEffectApplier() {

‎worldedit-core/src/main/java/com/sk89q/worldedit/util/SideEffectSet.java‎

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,12 @@
1919

2020
package com.sk89q.worldedit.util;
2121

22+
import com.google.common.base.Verify;
2223
import com.google.common.collect.ImmutableMap;
23-
import com.google.common.collect.Maps;
24+
import com.google.common.collect.Sets;
2425

2526
import java.util.Arrays;
26-
import java.util.EnumMap;
27+
import java.util.EnumSet;
2728
import java.util.Map;
2829
import java.util.Set;
2930
import java.util.function.Function;
@@ -37,33 +38,73 @@ public class SideEffectSet {
3738
.collect(Collectors.toMap(Function.identity(), state -> SideEffect.State.OFF))
3839
);
3940

40-
private final Map<SideEffect, SideEffect.State> sideEffects;
41-
private final Set<SideEffect> appliedSideEffects;
42-
private final boolean appliesAny;
41+
static {
42+
Verify.verify(
43+
SideEffect.State.values().length == 3,
44+
"Implementation requires specifically 3 values in the SideEffect.State enum"
45+
);
46+
int maxEffects = Integer.SIZE / 2;
47+
Verify.verify(
48+
SideEffect.values().length <= maxEffects,
49+
"Implementation requires less than " + maxEffects + " side effects"
50+
);
51+
Verify.verify(
52+
SideEffect.State.OFF.ordinal() == 0,
53+
"Implementation requires SideEffect.State.OFF to be the first value"
54+
);
55+
}
56+
57+
private static int shift(SideEffect effect) {
58+
return effect.ordinal() * 2;
59+
}
60+
61+
private static int computeSideEffectsBitmap(Map<SideEffect, SideEffect.State> sideEffects) {
62+
int sideEffectsBitmap = 0;
63+
for (SideEffect effect : SideEffect.values()) {
64+
SideEffect.State state = sideEffects.getOrDefault(effect, effect.getDefaultValue());
65+
sideEffectsBitmap |= (state.ordinal() << shift(effect));
66+
}
67+
return sideEffectsBitmap;
68+
}
69+
70+
/**
71+
* Side-effects and state are encoded into this field, 2 bits per side-effect. Least-significant bit is first.
72+
*/
73+
private final int sideEffectsBitmap;
74+
private final Set<SideEffect> sideEffectsToApply;
4375

4476
public SideEffectSet(Map<SideEffect, SideEffect.State> sideEffects) {
45-
this.sideEffects = Maps.immutableEnumMap(sideEffects);
46-
47-
appliedSideEffects = sideEffects.entrySet()
48-
.stream()
49-
.filter(entry -> entry.getValue() != SideEffect.State.OFF)
50-
.map(Map.Entry::getKey)
51-
.collect(Collectors.toSet());
52-
appliesAny = !appliedSideEffects.isEmpty();
77+
this(computeSideEffectsBitmap(sideEffects));
78+
}
79+
80+
private SideEffectSet(int sideEffectsBitmap) {
81+
this.sideEffectsBitmap = sideEffectsBitmap;
82+
var sideEffectsToApply = EnumSet.noneOf(SideEffect.class);
83+
for (SideEffect effect : SideEffect.values()) {
84+
if (shouldApply(effect)) {
85+
sideEffectsToApply.add(effect);
86+
}
87+
}
88+
this.sideEffectsToApply = Sets.immutableEnumSet(sideEffectsToApply);
5389
}
5490

5591
public SideEffectSet with(SideEffect sideEffect, SideEffect.State state) {
56-
Map<SideEffect, SideEffect.State> entries = this.sideEffects.isEmpty() ? Maps.newEnumMap(SideEffect.class) : new EnumMap<>(this.sideEffects);
57-
entries.put(sideEffect, state);
58-
return new SideEffectSet(entries);
92+
int mask = 0b11 << shift(sideEffect);
93+
int newState = (state.ordinal() << shift(sideEffect)) & mask;
94+
int newBitmap = (sideEffectsBitmap & ~mask) | newState;
95+
return new SideEffectSet(newBitmap);
5996
}
6097

6198
public boolean doesApplyAny() {
62-
return this.appliesAny;
99+
return sideEffectsBitmap != 0;
63100
}
64101

65102
public SideEffect.State getState(SideEffect effect) {
66-
return sideEffects.getOrDefault(effect, effect.getDefaultValue());
103+
return SideEffect.State.values()[getRawState(effect)];
104+
}
105+
106+
private int getRawState(SideEffect effect) {
107+
return (sideEffectsBitmap >> shift(effect)) & 0b11;
67108
}
68109

69110
/**
@@ -77,11 +118,11 @@ public SideEffect.State getState(SideEffect effect) {
77118
* @return Whether it should apply
78119
*/
79120
public boolean shouldApply(SideEffect effect) {
80-
return getState(effect) != SideEffect.State.OFF;
121+
return getRawState(effect) != 0;
81122
}
82123

83124
public Set<SideEffect> getSideEffectsToApply() {
84-
return this.appliedSideEffects;
125+
return sideEffectsToApply;
85126
}
86127

87128
public static SideEffectSet defaults() {

‎worldedit-core/src/test/java/com/sk89q/worldedit/util/SideEffectSetTest.java‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
/*
2+
* WorldEdit, a Minecraft world manipulation toolkit
3+
* Copyright (C) sk89q <http://www.sk89q.com>
4+
* Copyright (C) WorldEdit team and contributors
5+
*
6+
* This program is free software: you can redistribute it and/or modify
7+
* it under the terms of the GNU General Public License as published by
8+
* the Free Software Foundation, either version 3 of the License, or
9+
* (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
14+
* GNU General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU General Public License
17+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
18+
*/
19+
20+
package com.sk89q.worldedit.util;
21+
22+
import com.google.common.base.Preconditions;
23+
import com.google.common.collect.Maps;
24+
import org.junit.jupiter.api.Test;
25+
26+
import java.util.Arrays;
27+
import java.util.EnumSet;
28+
import java.util.Map;
29+
import java.util.Set;
30+
import java.util.function.Function;
31+
import java.util.stream.Collectors;
32+
33+
import static org.junit.jupiter.api.Assertions.assertEquals;
34+
35+
public class SideEffectSetTest {
36+
private static void assertAppliesWithState(Map<SideEffect, SideEffect.State> expected, SideEffectSet set) {
37+
Preconditions.checkArgument(
38+
expected.keySet().containsAll(EnumSet.allOf(SideEffect.class)),
39+
"Expected map must contain all side effects"
40+
);
41+
42+
Set<SideEffect> appliedSet = expected.entrySet().stream()
43+
.filter(e -> e.getValue() != SideEffect.State.OFF)
44+
.map(Map.Entry::getKey)
45+
.collect(Collectors.toSet());
46+
assertEquals(appliedSet, set.getSideEffectsToApply());
47+
assertEquals(!appliedSet.isEmpty(), set.doesApplyAny());
48+
for (SideEffect effect : SideEffect.values()) {
49+
assertEquals(
50+
appliedSet.contains(effect), set.shouldApply(effect), "Does not apply expected effect: " + effect
51+
);
52+
assertEquals(
53+
expected.get(effect), set.getState(effect),
54+
"Does not have expected state for effect: " + effect
55+
);
56+
}
57+
}
58+
59+
private static Map<SideEffect, SideEffect.State> initStateMap(Function<SideEffect, SideEffect.State> stateFunction) {
60+
return Arrays.stream(SideEffect.values()).collect(Collectors.toMap(Function.identity(), stateFunction));
61+
}
62+
63+
@Test
64+
public void defaults() {
65+
assertAppliesWithState(
66+
initStateMap(SideEffect::getDefaultValue),
67+
SideEffectSet.defaults()
68+
);
69+
}
70+
71+
@Test
72+
public void noneExposed() {
73+
assertAppliesWithState(
74+
initStateMap(effect -> {
75+
if (effect.isExposed()) {
76+
return SideEffect.State.OFF;
77+
} else {
78+
return effect.getDefaultValue();
79+
}
80+
}),
81+
SideEffectSet.none()
82+
);
83+
}
84+
85+
@Test
86+
public void allOn() {
87+
Map<SideEffect, SideEffect.State> expected = initStateMap(effect -> SideEffect.State.ON);
88+
assertAppliesWithState(
89+
expected,
90+
new SideEffectSet(expected)
91+
);
92+
}
93+
94+
@Test
95+
public void allDelayed() {
96+
Map<SideEffect, SideEffect.State> expected = initStateMap(effect -> SideEffect.State.DELAYED);
97+
assertAppliesWithState(
98+
expected,
99+
new SideEffectSet(expected)
100+
);
101+
}
102+
103+
@Test
104+
public void allOff() {
105+
Map<SideEffect, SideEffect.State> expected = initStateMap(effect -> SideEffect.State.OFF);
106+
assertAppliesWithState(
107+
expected,
108+
new SideEffectSet(expected)
109+
);
110+
}
111+
112+
@Test
113+
public void with() {
114+
Map<SideEffect, SideEffect.State> expected = initStateMap(SideEffect::getDefaultValue);
115+
SideEffectSet set = SideEffectSet.defaults();
116+
117+
for (SideEffect effect : SideEffect.values()) {
118+
for (SideEffect.State state : SideEffect.State.values()) {
119+
expected = Maps.transformEntries(expected, (e, s) -> e == effect ? state : s);
120+
set = set.with(effect, state);
121+
assertAppliesWithState(expected, set);
122+
}
123+
}
124+
}
125+
}

0 commit comments

Comments
 (0)