Additional methods in relation to create #179
Conversation
…o create 6 compile errors
| default void onNeighborChange(BlockState state, LevelReader level, BlockPos pos, BlockPos neighbor) { | ||
| //do nothing by default | ||
| } |
There was a problem hiding this comment.
This method exists in NeighborChangeListeningBlock
There was a problem hiding this comment.
Oh i missed that.
Also, it doesnt seem like NeighborChangeListeningBlock is implemented anywhere in the main class hierarchy for blocks (i.e. Block.class). This differs to forge where these 2 methods are effectively applied to every block via IForgeBlock and IForgeBlockState. Wouldn't this mean that Forge mods being ported to Fabric will have a common bug where a block class defines onNeighborChange but does not implement the NeighborChangeListeningBlock interface leading to blocks not updating when they should be on Fabric?
I realise that Override annotation could force a compile error but Java doesnt force that annotation so there will be cases where it isnt used and so the bug will be undetected until someone spots it in game.
How can this be avoided (or how is this avoided)
There was a problem hiding this comment.
Also, I did move to NeighborChangeListeningBlock but I think my point still stands (unless im misunderstanding something, still new to this). Already had to fix it for the vanilla comparator
.../java/io/github/fabricators_of_create/porting_lib/extensions/extensions/BlockExtensions.java
Outdated
Show resolved
Hide resolved
...ain/java/io/github/fabricators_of_create/porting_lib/entity/extensions/EntityExtensions.java
Show resolved
Hide resolved
…eakChanges to it), updated Comparator block to fix bug where it wont update via the vault
|
I've moved to using NeighbourListeningBlock. It also fixes the vanilla comparator not updating when the vault inventory updates by having the comparator also implement the NeighborListeningBlock (done this way to mimic how forge does it). |
|
I forgot to mention that I already did a PR on the Create Fabric repo Fabricators-of-Create/Create#1794 basically fixes the same errors. However, your approach might be slightly better |
|
Also I don't know if
Having the comparator implement NeighborListeningBlock will probably make it update twice for normal updates using Level#updateNeighborForOutputSignal |
Yes I did see that. I feel like it makes more sense to put it in the Porting lib. Most of the errors are due to differences between forge and fabric which is sort of the point of this library. |
Possibly. Atm I am just aiming to just keep it similar to what Forge does. Before that patch the comparator would not update when the vault inventory updated. |
...c/main/java/io/github/fabricators_of_create/porting_lib/entity/mixin/common/EntityMixin.java
Outdated
Show resolved
Hide resolved
…eted duplicate implementation of updateFluidOnEyes
…parators which now implement NeighborChangeListeningBlock
Ive updated that method in 0f1b608 to try and avoid that now |
| @Mixin(ComparatorBlock.class) | ||
| public class ComparatorBlockMixin implements ComparatorBlockExtension { | ||
| @Override | ||
| public void onNeighborChange(BlockState state, LevelReader world, BlockPos pos, BlockPos neighbor) { |
There was a problem hiding this comment.
Porting Lib isn't meant to override vanilla like this. All extensions are meant to be added to modded blocks that want the behavior
There was a problem hiding this comment.
Hi. The main issue is that forge adds this method to IForgeBlock interface which the comparator then implements in forge. I wanted to try and mimic this behaviour as the otherwise inconsistent implementation of onNeighbourChange in forge and fabric lead to the comparator missing updates from block entities which only call onNeighbourChange.
Otherwise, forge to fabric ports have to manually account for the fact that not all blocks implement NeighborChangeListeningBlock and then send those updates manually (albeit i think this will only really be an issue with updates to vanilla blocks since modded blocks should already have implemented this method if they are listening to neighbour changes).
As an example, this was an issue with the create mod vault not updating comparators.
Ig the other way would be to have a neighbour notify interface which when implemented can include calls to neighbour change listeners + an additional line for notifying neighbouring blocks of type comparator.
There was a problem hiding this comment.
With that, the comparator mixin can be removed. If thats preferrable i can do that
| * and {@link LevelMixin#port_lib$updateNeighbourForOutputSignal2(BlockPos, Block, CallbackInfo, BlockPos, BlockState)} | ||
| * which call {@link NeighborChangeListeningBlock#onNeighborChange(BlockState, LevelReader, BlockPos, BlockPos)} | ||
| */ | ||
| @Redirect(method = "updateNeighbourForOutputSignal", |
There was a problem hiding this comment.
Redirecting vanilla code for this isn't what Porting Lib is for
There was a problem hiding this comment.
I can remove this as well in relation to the suggested change in #179 (comment)
| throw new RuntimeException("this should be overridden via mixin. what?"); | ||
| } | ||
|
|
||
| default FluidType getEyeInFluidType() { |
There was a problem hiding this comment.
Methods that don't have a unique method signature like this means that it will conflict with other mods and need to be prefixed.
added additional methods to try and fix compile errors in create fabric. Were put here as it could be useful outside of create too: