- 
                Notifications
    
You must be signed in to change notification settings  - Fork 101
 
feat: omnishape compat to add framable pipes/wires #498
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
base: main
Are you sure you want to change the base?
Conversation
…rames can now be placed on Galacticraft wires as covers.
…al compat works nicely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things
| 
               | 
          ||
| modCompileOnly("mezz.jei:jei-$minecraftVersion-fabric-api:$jeiVersion") | ||
| 
               | 
          ||
| modCompileOnly("dev.maximus:omnishape:1.2.4") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a property/placeholder for the version (like the other dependencies).
| if ((mixinClassName.contains("ExplosionDamageCalculatorMixin") || mixinClassName.contains("ServerPlayerGameModeMixin")) && !OMNISHAPE_LOADED) { | ||
| return false; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit nitpicky but I think this is clearer.
| if ((mixinClassName.contains("ExplosionDamageCalculatorMixin") || mixinClassName.contains("ServerPlayerGameModeMixin")) && !OMNISHAPE_LOADED) { | |
| return false; | |
| } | |
| if ((mixinClassName.contains("ExplosionDamageCalculatorMixin") || mixinClassName.contains("ServerPlayerGameModeMixin"))) { | |
| return OMNISHAPE_LOADED; | |
| } | 
It would probably be a good idea to include the omnishape. package prefix in the test (since the mixins are named after their targets)
| protected @NotNull VoxelShape getShape(BlockState state, BlockGetter world, BlockPos pos, CollisionContext context) { | ||
| if (world.getBlockEntity(pos) instanceof Connected connected) { | ||
| return this.shapes[generateAABBIndex(connected)]; | ||
| VoxelShape baseShape = Shapes.empty(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this theoretically shouldn't ever occur, it's probably better to fallback to this.shapes[0] (as before) rather than empty.
| import static dev.galacticraft.mod.api.block.PipeShapedBlock.preciseHit; | ||
| 
               | 
          ||
| @Mixin(MultiPlayerGameMode.class) | ||
| public abstract class MultiPlayerGameModeMixin { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be filtered in the mixin plugin as well
| } | ||
| 
               | 
          ||
| @Nullable | ||
| private OmnishapeData frameOverlay = null; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Omnishape isn't loaded OmnishapeData won't exist (I'm pretty sure a field will always cause a class load but idk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm maybe it was just due to the debug environement but that never threw an issue when i removed the mod from my dev environment. i wasnt 100% sure how to do it without that but after your recent suggestion of making the omnishape api have its own system for blocks like that then yes it should be possible to remove that.
| return new Matrix3f(); | ||
| } | ||
| 
               | 
          ||
| public VoxelShape getOrCreateHitbox(Matrix3f rotationMatrix) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like all current uses of this method provide the value of the getRotationMatrix method above as the argument, so maybe you could streamline this a bit.
| 
               | 
          ||
| if (hitFrame) { | ||
| // Only break frame overlay | ||
| //level.setBlock(pos, wire.getOverlay().camouflage(), 3); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to keep this comment?
| 
               | 
          ||
| // Remove the overlay but keep the wire | ||
| wire.setOverlay(null); | ||
| level.sendBlockUpdated(pos, wire.getBlockState(), wire.getBlockState(), 3); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Block.UPDATE_ALL instead of 3 is a bit more readable.
Added compat between Omnishape and Galacticraft 5.