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

Expand logging capabilities #197

Merged
merged 35 commits into from
Feb 11, 2024
Merged

Expand logging capabilities #197

merged 35 commits into from
Feb 11, 2024

Conversation

nikijaz
Copy link
Contributor

@nikijaz nikijaz commented Mar 9, 2023

This PR expands logging capabilities and fixes some bugs. This also includes changes from #119 by Altirix.

Added logging of:

@nikijaz nikijaz requested a review from a team as a code owner March 9, 2023 16:50
Copy link
Contributor

@DrexHD DrexHD left a comment

Choose a reason for hiding this comment

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

Wow! This is a pretty big PR with a lot of changes.
I have added some small reviews to some parts of the code. I am putting some other things I would like to mention that didn't fit a specific piece of code here:

  • @ModifyArgs is used in a couple of Mixins to get easy access to arguments. According to the Java Doc of ModifyArgs, it is pretty inefficient and this isn't really its intended use-case. I suggest using LocalCapture instead.
  • This PR added a couple of mixins for plants breaking. Their source is Gravity, I think it would be great to add a separate source type for this, to allow better user blacklists (This would apply to CactusBlockMixin, SugarCaneBlockMixin, BambooBlockMixin, ChorusPlantBlock, and PointedDripstoneBlockMixin)
  • This is probably a design choice, but since entity modify actions store entity positions, the entity is moved back to the action pos when rolling back (even if the position doesn't really matter, like dyeing).
  • Item frame logging didn't work properly when I tried rolling back @equip actions
  • When placing carved pumpkins (and probably wither skeleton skull) it first logs the block being broken by snowman spawning and then tries to log the placed block, but since it's no longer there it logs placing air (so the order is incorrect and the placed block is wrong)
  • ItemDropActionType and ItemPickUpActionType should implement rollback / restore

Thanks for the PR!

@suerion
Copy link

suerion commented Mar 15, 2023

I think on this Pull Request, there should also be the #202 projectile shoot event

@PotatoPresident PotatoPresident linked an issue Mar 16, 2023 that may be closed by this pull request
2 tasks
@nikijaz nikijaz requested a review from DrexHD March 31, 2023 18:21
@nikijaz
Copy link
Contributor Author

nikijaz commented Mar 31, 2023

@ModifyArgs is used in a couple of Mixins to get easy access to arguments. According to the Java Doc of ModifyArgs, it is pretty inefficient and this isn't really its intended use-case. I suggest using LocalCapture instead.

Fixed!

This PR added a couple of mixins for plants breaking. Their source is Gravity, I think it would be great to add a separate source type for this, to allow better user blacklists (This would apply to CactusBlockMixin, SugarCaneBlockMixin, BambooBlockMixin, ChorusPlantBlock, and PointedDripstoneBlockMixin)

I honestly don't see much point in it. After all, the remaining parts of the plants are destroyed just by gravity, which is exactly what fits this source.

This is probably a design choice, but since entity modify actions store entity positions, the entity is moved back to the action pos when rolling back (even if the position doesn't really matter, like dyeing).

Fixed, now only custom entity data is rolled back.

Item frame logging didn't work properly when I tried rolling back @equip actions

When using readNbt() for an ItemFrame entity with an existing item stack in it, if the new NBT does not have an item stack, it will not be removed from the entity. I made a simple workaround for this that should work.

When placing carved pumpkins (and probably wither skeleton skull) it first logs the block being broken by snowman spawning and then tries to log the placed block, but since it's no longer there it logs placing air (so the order is incorrect and the placed block is wrong)

I don't know how to fix it. onPlaced() is processed before the final installation of the block, so I have no idea.

ItemDropActionType and ItemPickUpActionType should implement rollback / restore

Implemented, but without returning items to the player's inventory, as I consider it unnecessary.

Copy link
Contributor

@DrexHD DrexHD left a comment

Choose a reason for hiding this comment

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

The new changes look pretty good 👍. I have left some more change request on some minor issues.

I have also noticed an issue, when rolling back an armorstand that was first equipped and then killed. (The killing gets rolled back, but equipping doesn't, if both are rolled back in one operation)! I don't know what might cause this atm.
I think you should also remove the code duplication in ItemDropActionType and ItemPickUpActionType rollback/restore.

Sorry for the slow responses, I was/am kind of busy currently

@AphoticGoblin
Copy link

This isn't compatible with Goodall. There's an item called the Animal Crate that can capture animals, that isnt logged by this.
image_2023-04-30_111441409

nikijaz added 5 commits May 7, 2023 19:51
# Conflicts:
#	src/main/java/com/github/quiltservertools/ledger/mixin/SnowGolemEntityMixin.java
#	src/main/kotlin/com/github/quiltservertools/ledger/actions/ItemChangeActionType.kt
@nikijaz nikijaz requested a review from DrexHD July 3, 2023 10:14
DrexHD
DrexHD previously approved these changes Jul 3, 2023
Copy link
Contributor

@DrexHD DrexHD left a comment

Choose a reason for hiding this comment

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

Looks good to me!

# Conflicts:
#	src/main/resources/ledger.mixins.json
@Dev0Louis
Copy link

Any news on this?

@Rozbiynk
Copy link

any updates?

DrexHD
DrexHD previously approved these changes Oct 21, 2023
@transcental
Copy link

Would it be possible for this to be merged soon? Had an issue on my server that this would've solved but instead I had to go and move region files from an old backup as ledger hadn't logged the destruction.

@Rozbiynk
Copy link

i hope it will have 1.20.1 version

@TheDeathlyCow
Copy link
Contributor

TheDeathlyCow commented Dec 28, 2023

I've done some testing and can confirm that this branch works on 1.20.2 by just merging in 1.2.9.

There are, however, a couple of issues when merging in update to 1.20.4.

First, The setBlockState target for the injection TrapdoorBlockMixin#logTrapdoorInteraction no longer exists. I think this can be fixed by changing the target to the flip call instead, as that seems to be the replacement.

Second, the BedBlockMixin has some invalid descriptors. It seems that onBreak now returns a BlockState so just changing the callback info param to CallbackInfoReturnable<BlockState> fixes that.

Those are the only 2 errors I encountered with 1.20.4. Fixing them locally gets the mod running, and everything still seems to work.

Hope this PR gets merged soon, it has some really nice features!

@PotatoPresident
Copy link
Member

I can't help but feel that sources is being misused, but I don't really have a better alternative other than making a ton of different action types.

@PotatoPresident PotatoPresident merged commit 8b5522c into QuiltServerTools:master Feb 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment