Rob23
- Rob23
- JIRAUSER743209
- Europe/Stockholm
- Yes
- No
MC-259032
Normal
Result of /data get can underflow/data produces
/data produces/data produces a positive result for low negative numbers
When you apply a /data get command to a value that is smaller than -2147483648 (Integer.MIN_VALUE), it returns a result of 2147483647 (Integer.MAX_VALUE).
Steps to Reproduce:
1. Create annbttag with a value below -2147483648, e.g./data modify storage test:test value set value -3.0e9d2. Run /data get and store its result using /execute store:
/execute store result storage test:test result int 1 run data get storage test:test value3. Retrieve the result
/data get storage test:test resultThe result will be 2147483647 (a positive integer), while it should be a negative integer like -2147483648.
When you apply a /data get command to a value that is smaller than -2147483648 (-231, the lowest int value), it returns a result of 2147483647 (231-1, the highest int value).
Explanation:
For the result of the /data get command, the following function is used to convert into an integer:
public static int floor(double x) { int i = (int) x; return x < i ? x - 1 : x; }The first line in the function removes the digits after the decimal point. Since the int type only provides the fixed range between -2147483648 (-231) and 2147483647 (231-1), any values above or below are reduced to this maximum or minimum, meaning that any values below -231 are converted to exactly -231.
The problem then occurres in the second line, which will, in the case of such a low number, say yes, x is smaller than -231 and try to subtract 1. Since the number is already the smallest possible, this will cause the number to underflow and wrap around to the maximum value (231-1).
Implications:
A usually good approach to determining whether a numeric tag is negative is to take its /data get result and determine whether that is negative. This approach stops working because of this bug. In fact, there is currently no simple approach that works for all numbers (except for using number to string conversion in function macros, which is much slower and tedious).
Possible fix:
Change the implementation of net.minecraft.util.Mth.floor to this:
public static int floor(double x) { return (int) Math.floor(x); }This produces the desired output and fixes the bug. For any performance considerations, it seems to be approximately as fast as the current approach, maybe slightly slower.
Steps to Reproduce:
1. Create an NBT tag with a value below -231, e.g./data modify storage test:test value set value -3.0e9d2. Run /data get and store its result using /execute store:
/execute store result storage test:test result int 1 run data get storage test:test value3. Retrieve the result
/data get storage test:test resultThe result will be 2147483647 (a positive integer), while it should be a negative integer like -2147483648.
Setting the generic.gravity attribute of an entity to a value <= 0.0030612244302160993 (~ 3/980) will make them unable to fall without downwards momentum to start with and forces their OnGround tag to be false, even when the entity is on the ground. To players, this means that you can't jump and have inertia similarly to flying in creative mode.
Steps to reproduce:
- Set your generic.gravity attribute to a value below 0.0030612244302160993, e.g. 0.003:
/attribute @s minecraft:generic.gravity base set 0.003- Go onto the ground and check your OnGround tag:
/data get entity @s OnGround- Try to move around and jump
- Go into creative mode and fly into the air and then stop flying.
Expected behavior:
- The OnGround tag should be true (1b).
- You should be able to move around without inertia and jump.
- You should be able to fall while in the air.
Actual behavior:
- The OnGround tag is false (0b).
- You feel a lot of inertia while moving, similarly to flying in creative mode
- You stay in the air and can't fall down.
Code analysis:
net.minecraft.world.entity.LivingEntitypublic void aiStep() { // ... if (!isEffectiveAi()) { setDeltaMovement(getDeltaMovement().scale(0.98)); // Line 2713 } // ... if (Math.abs(motion.x) < 0.003) { // Line 2724 xMotion = 0; } if (Math.abs(motion.y) < 0.003) { yMotion = 0; } if (Math.abs(motion.z) < 0.003) { zMotion = 0; } // ... }The motion of the entity is first multiplied by 0.98 and if the result in a particular direction turns out to be less than 0.003, the motion in that direction is removed. If this happens to be the negative y direction, the falling gets negated.
Setting the generic.gravity attribute of an entity to a value <= 0.0030612244302160993 (~ 3/980) will make them unable to fall without downwards momentum to start with and forces their OnGround tag to be false, even when the entity is on the ground. To players, this means that you can't jump and have inertia similarly to flying in creative mode.
Steps to reproduce:
- Set your generic.gravity attribute to a value below 0.0030612244302160993, e.g. 0.003:
/attribute @s minecraft:generic.gravity base set 0.003- Go onto the ground and check your OnGround tag:
/data get entity @s OnGround- Try to move around and jump
- Go into creative mode and fly into the air and then stop flying.
Expected behavior:
- The OnGround tag should be true (1b).
- You should be able to move around without inertia and jump.
- You should be able to fall while in the air.
Actual behavior:
- The OnGround tag is false (0b).
- You feel a lot of inertia while moving, similarly to flying in creative mode
- You stay in the air and can't fall down.
Code analysis:
net.minecraft.world.entity.LivingEntitypublic void aiStep() { // ... if (!isEffectiveAi()) { setDeltaMovement(getDeltaMovement().scale(0.98)); // Line 2713 } // ... if (Math.abs(motion.x) < 0.003) { // Line 2724 xMotion = 0; } if (Math.abs(motion.y) < 0.003) { yMotion = 0; } if (Math.abs(motion.z) < 0.003) { zMotion = 0; } // ... }The motion of the entity is first multiplied by 0.98 and if the result in a particular direction turns out to be less than 0.003, the motion in that direction is removed. If this happens to be the negative y direction, the falling gets negated.
Potential fix:
Disable the cut-off for the y direction if the entity has gravity:
if (Math.abs(motion.y) < 0.003 && getGravity() == 0) { yMotion = 0; }
External inventory changes (e.g. by picking up items) are not registered on the client if you are in creative mode inventory and are not in the Survival Inventory tab, causing a desync.
Steps to reproduce:
- Fill your hotbar with items.
- Go into the creative mode inventory and go to any tab, but the inventory tab (e.g. the search tab).
- While in the tab, take any other item and throw it onto the ground.
- Wait until you pick it up and observe whether or not the item appears in the inventory.
Expected behavior:
The item is picked up and should show up in the inventory.Actual behavior:
The item does not appear in the inventory. However, if you try to remove that particular item, e.g. using /clear @s <item>, the server will report that the item indeed was in the inventory.Code analysis:
net.minecraft.client.multiplayer.ClientPacketListenerpublic void handleContainerSetSlot(ClientboundContainerSetSlotPacket packet) { // ... boolean conditional = false; // Line 1200 if (minecraft.screen instanceof CreativeModeInventoryScreen x) { // Line 1202 conditional = !x.isInventoryOpen(); // Line 1203 } // 0 means player inventory if (packet.getContainerId() == 0 && InventoryMenu.isHotbarSlot(slotId)) { // ... player.inventoryMenu.setItem(slotId, packet.getStateId(), item); } else if (packet.getContainerId() == player.containerMenu.containerId && (packet.getContainerId() != 0 || !conditional)) { player.containerMenu.setItem(slotId, packet.getStateId(), item); } }This method is responsible for all of the single slot synchronization on the client side.
Because of it, inventory changes are almost completely ignored if they occur in a non-hotbar slot (first condition) and you are in the creative mode inventory (Line 1202), but not in the "Survival Inventory" tab (Line 1203). (why?)Potential fix:
Just remove those weird conditions. Why do they even exist in the first place?public void handleContainerSetSlot(ClientboundContainerSetSlotPacket packet) { // ... if (packet.getContainerId() == 0) { // ... player.inventoryMenu.setItem(slotId, packet.getStateId(), item); } else if (packet.getContainerId() == player.containerMenu.containerId) { player.containerMenu.setItem(slotId, packet.getStateId(), item); } }Also, because this is a synchronization step, the client should be able to be confident about that particular remote slot, shouldn't it? So please also consider changing this:
net.minecraft.world.inventory.AbstractContainerMenupublic void setItem(int slotId, int stateId, ItemStack item) { getSlot(slotId).set(item); // Line 593this.remoteSlots.set(slotId, item);// New linethis.stateId = stateId; // Line 594 }That is the setItem method from above, by the way.
External inventory changes (e.g. by picking up items) are not registered on the client if you are in creative mode inventory and are not in the Survival Inventory tab, causing a desync.
Steps to reproduce:
- Fill your hotbar with items.
- Go into the creative mode inventory and go to any tab, but the inventory tab (e.g. the search tab).
- While in the tab, take any other item and throw it onto the ground.
- Wait until you pick it up and observe whether or not the item appears in the inventory.
Expected behavior:
The item is picked up and should show up in the inventory.Actual behavior:
The item does not appear in the inventory. However, if you try to remove that particular item, e.g. using /clear @s <item>, the server will report that the item indeed was in the inventory.Code analysis:
net.minecraft.client.multiplayer.ClientPacketListenerpublic void handleContainerSetSlot(ClientboundContainerSetSlotPacket packet) { // ... boolean conditional = false; // Line 1200 if (minecraft.screen instanceof CreativeModeInventoryScreen x) { // Line 1202 conditional = !x.isInventoryOpen(); // Line 1203 } // 0 means player inventory if (packet.getContainerId() == 0 && InventoryMenu.isHotbarSlot(slotId)) { // ... player.inventoryMenu.setItem(slotId, packet.getStateId(), item); } else if (packet.getContainerId() == player.containerMenu.containerId && (packet.getContainerId() != 0 || !conditional)) { player.containerMenu.setItem(slotId, packet.getStateId(), item); } }This method is responsible for all of the single slot synchronization on the client side.
Because of it, inventory changes are almost completely ignored if they occur in a non-hotbar slot (first condition) and you are in the creative mode inventory (Line 1202), but not in the "Survival Inventory" tab (Line 1203). (why?)Potential fix:
Just remove those weird conditions. Why do they even exist in the first place?public void handleContainerSetSlot(ClientboundContainerSetSlotPacket packet) { // ... if (packet.getContainerId() == 0) { // ... player.inventoryMenu.setItem(slotId, packet.getStateId(), item); } else if (packet.getContainerId() == player.containerMenu.containerId) { player.containerMenu.setItem(slotId, packet.getStateId(), item); } }Also, because this is a synchronization step, the client should be able to be confident about that particular remote slot, shouldn't it? So please also consider changing this:
net.minecraft.world.inventory.AbstractContainerMenupublic void setItem(int slotId, int stateId, ItemStack item) { getSlot(slotId).set(item); // Line 593 setRemoteSlot(slotId, item); // New line this.stateId = stateId; // Line 594 }That is the setItem method from above, by the way.
Well, not exactly. Actually, when converting a double into an int directly, Java will just return the minimum possible integer if the double is too small to fit into a int. But Minecraft uses a special method to round down doubles into ints, which returns the maximum possible integer for both too low and too high values. In my opinion it should return the maximum possible integer for too high and the minimum possible integer for too low numbers.
Updated the description to better explain the issue.
This behavior is in my opinion very awkward because it contradicts the initial idea of how functions should work (especially because /return exists now). A function (usually) returns one value at the end of execution. /execute store result ... run function ... is expected to store that return value inside of a score/NBT path/etc., but instead what it does is store all the different return values from the individual commands (and, in fact, from the original function call which returns 0) one after the other inside of the store location and the return values coincidentally match because the last command was the return command and overrode the previous value. This is also the reason /return run function doesn't work (
MC-264595): The original function call returns 0 and the return run result consumer immediately aborts the function before actually running a single command. And even if the original function call wouldn't activate the result consumer, the first command in the function would and only one command would be executed.How to fix?
To fix this issue, the result consumer would need to be detached when a function call happens and replaced by a return value consumer. When the function then returns (or finishes maybe), the return value consumer should be executed. One issue might persist, specifically, the result consumer would also need to be removed from the original command context as the function call happens and I am not sure if the command system is currently equipped for that (I think that would require a redirect in the current system).
I think this has something to do with
MC-262027, the basic problem is that the result consumer of /return run is executed when the /function command is executed.The main problem about the current implementation is that /execute if|unless function ... assumes that the function being called will be finished after executing ServerFunctionManager.execute(...). This is not the case when in a function because function calls are always postponed by design to prevent infinite recursion. That means that the naive approach of call the function(s), get the result, make a condition, done will not work in the current system. A possible different approach (when it is run in a function) would be to run the function(s) with an added return value consumer (assuming
MC-262027will be fixed). The return value consumer should take note of whether there was a non-zero return value and, if it notices all of the commands have been run, check the condition and potentially run the command. Problem: To my knowledge the current command system (brigadier) doesn't work like that. So essentially you would need to change the command system to make redirects more powerful unless there is a better solution (I mean, this is just a suggestion, after all). Generally, I can say that implementing this command would probably be much more complicated than whoever tried to implement it thought, but it would probably still be worth it: The execute if function command would probably be the easiest way to filter entities if predicates aren't enough.Should have been fixed in 23w41a.
Can confirm.
Just did a code analysis, here is a stack trace of where the problem seems to be coming from:
Block.pushEntitiesUp(...) (line 144)
EntityGetter.getEntities(Entity, AABB) (line 33)
Level.getEntities(Entity, AABB, Predicate) (line 676)
LevelEntityGetterAdapter.get(AABB, Consumer) (line 43)
EntitySectionStorage.getEntities(AABB, AbortableIterationConsumer) (line 122)
EntitySectionStorage.forEachAccessibleNonEmptySection(...) (line 65)
EntitySection.getEntities(AABB, AbortableIterationConsumer)
In the method EntitySection.getEntities(...) there is no check for whether the entity is a marker or display entity but only a check for the bounding box, where the empty bounding box of a marker or display entity seems to also be able to intersect with other bounding boxes. This also affects @e[dx=...,dy=...,dz=...] selectors in that they also pick up markers and display entities but I'm not sure whether that behavior should be changed or not.
I rather think the default entity predicate used in EntityGetter.getEntities should be changed to accommodate for markers and display entities as it is also used in other places, e.g. to determine whether endermen can place blocks somewhere, whether an armor stand or end crystal can be placed and which entities are moved away from explosions (note: markers and display entities won't move away, only the Motion tag is changed).
This is not quite a duplicate because MC-89178 is about all entities, which is not too important, but this issue is specifically for markers and display entities, which is important because markers and display entities should not have any game play relevant impact.
The issue is not completely gone, the success value now seems to be assigned correctly, however, it is still not possible to distinguish between a function that failed and a function that returned 0, the message is still "Function ... returned 0", which is misleading.
If anything, this is a feature request. Currently, to work around this, you can use function macros in data packs.
Did a code analysis, it seems like Minecraft just discards all attribute modifiers where the first two or the last two numbers of the UUID are 0:
This is intentional behavior: Any items that would usually stay in a crafting table are ejected to make sure the crafter is empty after crafting and ready to craft again. Leaving them inside would not make any sense because then the spaces are occupied and therefore in the way for the next crafting recipe.
Affects 24w03a
Duplicate of
MC-268362Code analysis for block breaking
In this code, the following operations are applied to the block breaking speed:
Step 1 can't be put into the attribute because it depends on the block you are breaking.
Steps 2 and 3 can be put into attribute modification due to being multiplication.
Steps 5 and 6 might be put into attribute modification, however they depend on the environment.
Note: Step 3 might be slightly difficult, since attribute modification is usually linear and here, exponential
Minecraft does not use the Bogosort algorithm, as seen here:
Minecraft does in fact use the default Java sorting algorithm provided by List.sort(...).
Quote directly taken from the documentation of List.sort:
"This implementation is a stable, adaptive, iterative mergesort that requires far fewer than n lg(n) comparisons when the input array is partially sorted, while offering the performance of a traditional mergesort when the input array is randomly ordered."
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/List.html#sort(java.util.Comparator)
The simplest way to confirm that Minecraft does in fact not use Bogosort is to try to sort bigger amounts of entities. If Minecraft would use the Bogosort algorithm, sorting 100 entities should take approximately 10^151^ longer than sorting 10 entities. However, 100 entities can in fact be sorted by Minecraft in very few time.
The only concern is that, when specifying a selector @e[limit=...,sort=nearest], Minecraft will sort all of the entities before applying the limit. This causes the sorting process to be much slower than necessary. For example an @e[limit=1,sort=nearest] selector will run in O(n log(n)) where n is the total amount of entities, where it could run in O(n) time, going through every entity once. This could be solved by using bubblesort if the limit is low in comparison to the amount of entities. The interesting fact about bubblesort is, that after k iterations, the last k elements will be correctly sorted. Therefore, you could apply k=limit iterations of bubblesort on the n entities, resulting in a time complexity of O(nk). This can be beneficial if the amount of entities is in the thousands and we only want 2 or 3 entities.
Conclusion: Minecraft doesn't use Bogosort, however the sorting process might be accelerated by using bubblesort if the amount of entities sorted is much higher than the wanted limit (e.g. 1000 compared to 3).
This is intentional behavior. In order to prevent this, you can modify the generic.fall_damage_multiplier and generic.safe_fall_distance attributes.
Code analysis for jump boost
The jump strength is here calculated as follows:
The problem is that the jump boost effect happens last and after the multiplication of two values that are dependent on the action and the environment.
A good way to observe this is to jump with a horse that has a generic.jump_strength of 0 and jump boost 10. This will cause the horse to always jump the same amount, whether you try to do a small jump or a high jump.
If this behavior should stay, it is pretty much impossible to put the jump boost into the attribute. If, however, the jump boost effect would apply within the attribute and the multipliers all come after, the behavior from before could not be recreated that easily and the jump strength will depend more on these other multipliers.
My statement from
MC-268417partially applies here: If Minecraft would use Bogosort, then running /say @e[c=100] should take longer than the heat death of the universe. However, as one can observe, it does not take that long and is done almost instantly.For swing duration:
This is currently not controlled by an attribute, it could theoretically be an attribute, but I don't think it would really be that important (could be generic.swing_duration with a base value of 6 and additive modifiers for haste and mining fatigue).
For Slow Falling and Levitation:
Slow falling is just a gravity override (gravity = 0.01) if the gravity was greater.
Levitation gradually increases the motion in the y direction, similarly to falling, however levitation causes 0.8 additional friction, causing the maximum velocity to be reached more quickly. Otherwise it can be modeled by gravity that is approximately -0.000926 times the levitation level, which reaches the same terminal velocity, but more slowly.
Despite what one might think, #guarded_by_piglins has nothing to do with using/opening the block. Piglins will always be angered by opening chests/barrels/..., no matter whether chests/barrels/... are considered #guarded_by_piglins or not. Therefore this is really the question of whether opening vaults should generally aggro piglins.
Problems from older versions will not be fixed, only the latest versions get updated.
Yes, exactly. #guarded_by_piglins is only responsible for aggroing piglins when destroying blocks.
Just did a quick code analysis and it seems like this bug could be solved like this:
A similar fix can also be applied to other places, specifically command blocks, containers, beacons, banners and enchanting tables.
An alternative way would be to make a method dedicated to retrieving a custom name, with the same code as above, that returns the custom name.
The fix comes from removing the custom name if the tag is not present. Through testing, this seems to work as expected, /data remove does remove the custom name, other types of changes also don't cause any issues.
I have attached a datapack to demonstrate this. To test this bug,
You will see the message "Revoked the advancement ..." which is incorrect because the entity was killed a player. Instead the message "Couldn't revoke advancement ... as they don't have it", indicating that the advancement was not triggered.
However, one might argue that this is not a bug. "killed_by_player" is meant as a condition for loot tables and not for advancements. In fact, not even the "player_killed_entity" criterion sets the "killed_by_player" condition.
That being said, I included a version of the advancement that checks for health being 0 instead of using "killed_by_player". This version can be found under test:hurt_by_player2 and does not trigger on kill like expected.
This is probably quasi-connectivity (
MC-108) and therefore intentional behaviorTo fix this, you probably need to remove the quotes around "true" and "false"