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.

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

Make chunk sections only convert vanilla air blocks to AIR #3535

Merged
merged 3 commits into from
Jan 28, 2024

Conversation

TelepathicGrunt
Copy link
Contributor

The Issue:

When a chunk section (16x16x16 area) is entirely made of blocks that return true for isAir method, vanilla will convert ALL blocks in that chunk section to Blocks.AIR regardless of what block it is. This does impact CAVE_AIR and VOID_AIR too.

If a mod has a modded air block returning true for isAir, they are getting this nuking behavior above without knowing it because of how hidden and inconsistent this behavior is. There is no use case for wanting this behavior for a block so no need to expose this for modders to opt into since no one will do so.

Vanilla bug report: https://bugs.mojang.com/browse/MC-232360

Solution 1: Tags:

I thought about this but given how widely and heavily used isAir checks are present in just about most mods, it would be impossible to get everyone to switch all of those checks to a tag check. Never will happen.

Solution 2: Change the ChunkSection to ignore modded air blocks:

Easiest solution. 2 mixins. And now all modded isAir blocks no longer will be converted into Air automatically by vanilla. It is safe for mods to mark their block as air and get all the isAir compatibility checks they want with other mods. Vanilla air block behavior is unchanged so CAVE_AIR and VOID_AIR still converts to AIR.

Why I want this:

I have Heavy Air and Windy Air block and I want them to be treated like air for all modded and vanilla isAir checks. I just don't want them magically disappearing from a chunk section because someone mined out the last solid block from the section somewhere else. Makes zero logical sense.

Copy link
Contributor

@apple502j apple502j left a comment

Choose a reason for hiding this comment

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

LGTM

@modmuss50
Copy link
Member

Although this is an observable deviation from a vanilla behavior, it is likely acceptable(?)

No, this should behave 100% the same for vanilla blocks if im understanding this corrrectly. Modded air blocks should have the different behaviour.

@apple502j
Copy link
Contributor

Ah yeah, whoops.

@modmuss50 modmuss50 added enhancement New feature or request last call If you care, make yourself heard right away! labels Jan 19, 2024
@Syst3ms
Copy link
Contributor

Syst3ms commented Jan 19, 2024

Has this been actually brought up with some of the folks working on Lithium? They'd be more qualified to tell what performance repercussions this could have.

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Jan 19, 2024

@Syst3ms Lithium devs are not the only people that know about optimizations. I already talked with several people who understand JVM optimizations and they said this is not an issue. The JVM is smart. The isOf methods are pure functions so the JVM will be able to optimize them all into a single call.

And Lithium mixins do not interfere here.
https://github.com/CaffeineMC/lithium-fabric/blob/b8f2818899bfcdae1bbf34991f2d3351e72c2f6c/src/main/java/me/jellysquid/mods/lithium/mixin/chunk/no_locking/ChunkSectionMixin.java#L20

https://github.com/CaffeineMC/lithium-fabric/blob/b8f2818899bfcdae1bbf34991f2d3351e72c2f6c/src/main/java/me/jellysquid/mods/lithium/mixin/util/block_tracking/ChunkSectionMixin.java#L28

@Devan-Kerman
Copy link
Contributor

Can there be a opposite check, incase a mod adds an isAir block but does want the nuking behavior? Eg. wishing well air from draylar's wishing well mod (idk if it's still around). It would somewhat make sense for the air to disappear if someone removed all the surrounding blocks since there's no well there anymore in the same way cave air gets destroyed because there's no cave there anymore

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Jan 20, 2024

@Devan-Kerman That would require adding a new method to FabricBlockSettings or so and that was discouraged for this PR because people wanted to wait to see what Mojang does with data driven blocks. Also, FabricBlockSettings is being deprecated soon too.

Also the nuking behavior is terrible with player expectations too. It's specifically about an entire chunk section empty which a player would have to know about the bounds in a technical sense and deliberately remove every block from there. If they have a torch or say string anywhere in chunk section, it won't nuke. So even in the case of vanilla itself, this nuking behavior is extremely unintuitive and you should not rely on it for removing your block if the environment changes imo. There's much better solutions out there you can do

@2No2Name
Copy link

I think this should be decided by thinking about whether fabric wants to change the behavior of modded air blocks depending on whether fabric is present. The vanilla behavior is bugged / not desirable, but it is the vanilla behavior. See also: Parrot monster spawner does not treat all air types equally https://bugs.mojang.com/browse/MC-232359 .
The fact that fabric does not modify any behavior is very valuable. The concrete behavior that is being changed here is also observable ingame with vanilla features, but even more with possible modded features (which is why the PR was created).

I would prefer anything that allows modders to specify which behavior they want, defaulting to the (broken) vanilla behavior. Forcing a change in behavior on mods that do not have a dependency on fabric-api does not sound like a good idea.

@TelepathicGrunt
Copy link
Contributor Author

@2No2Name just to clarify on top of what you said, this PR preserves the bug for vanilla blocks so there zero change in behavior right now with the PR in game with vanilla blocks. It only makes modded blocks not get the bugged behavior so that modders do not inadvertently get the bugged behavior without knowing about it.

@Ampflower
Copy link

There is still the point that mods not depending on Fabric API will have behaviour changes between with it vs. out; it's likely better as opt-in, or if possible, enforced on if it was depending on Fabric API.

@TelepathicGrunt
Copy link
Contributor Author

TelepathicGrunt commented Jan 20, 2024

mods not depending on Fabric API

I know this is about being thorough but generally, almost every fabric mod runs Fabric API. It’s even enforced in some launchers to always install. I cannot see ever a case where a player would actually be playing without Fabric API so mods without Fabric API is kinda a moot point.

Especially content mods with blocks. Heck, to make a custom block, you need resources and modded resources don’t load without Fabric API right? Thus any mods with blocks is forced to require Fabric API or else block assets/data files will not be loaded.

Edit: Figured it might be good for me to bring up the question of what possible use case could mods even want this nuking by chunk section behavior at all. Even if used for a marker for some wild modded purpose, it still makes no sense to have it delete based on chunk section.

Situation 1: Marker is in center of chunk section. All other blocks are remove from section. Marker is deleted with nothing around. Looks ok

Situation 2: Marker is at any edge of chunk section and the next chunk section is solid blocks. The marker is deleted due to its own section being empty. In world, marker was next to solid blocks, yet nuked. Looks weird as heck. Modder: “Why this happen???”

Situation 3: Marker is at any edge of chunk section, and there a few blocks on the opposite edge of the chunk away from marker. Marker refuses to be nuked. In world, marker looks like it has nothing around like situation 1 but it doesn’t disappear this time. modder: “wtf is going on???”

So in world, the nuking just feels random of whether it happens or not. As a result, whatever modded purpose you had for the marker will now also feel random whether it stays or disappears. It also means you cannot use it in say, giant rooms in structures that have a large space bigger than 16x16x16. It also can be randomly deleted based on where the structure is placed as depending on where in chunk section marker is placed like how situation 1 and 3 shows. There just doesn’t seem to be a valid modded use case for this nuking behavior that I can think of

@modmuss50 modmuss50 merged commit 3e2216c into FabricMC:1.20.4 Jan 28, 2024
modmuss50 pushed a commit that referenced this pull request Jan 28, 2024
* Make chunk sections only convert vanilla air blocks to AIR

* angry checkstyles calmed

* Comments added for future reference

(cherry picked from commit 3e2216c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants