Hey Nathan,

On our server we got a few moves that will create a cage and a wall out of blocks.
The problem with this is that when you use the move near an item frame, the blocks replace them.
This results in the dropping of the item frame alongside its contents.
Do you have any idea as to how this can be fixed (with your undo system)?

2 moves that have this problem:

earth_cage:
    actions:
        cast:
        -   class: CheckIfStanding
            allowed: earth
            actions:
            -   class: CustomProjectile
                actions:
                -   class: ChangeContext
                    source_at_target: true
                    actions:
                    -   class: Brush
                        brush: stone
                        actions:
                        -   class: Sphere
                            actions:
                            -   class: ModifyBlock
                        -   class: Stop
    effects:
        projectile:
        -   class: EffectSingle
            use_wand_location: false
            location: origin
            effectlib:
                class: SphereEffect
                radius: 0.5
                particles: 50
                duration: 500
                particle: block_crack
                material: stone
    parameters:
        range: 16
        target: LivingEntity
        destructible: destructible_replace
        transparent: transparent_no_water
        radius: 5
        thickness: 1
        velocity: 16
        reorient: true
        undo: 4000

earth_wall:
    actions:
        cast:
        -   class: CheckBlock
            allowed: earth
            actions:
            -   class: ChangeContext
                source_at_target: true
                actions:
                -   class: Brush
                    brush: stone
                    actions:
                    -   class: AreaOfEffect
                        radius: 2
                        actions:
                        -   class: Velocity
                            living_entity_speed: 0.5
                            push: 1
                    -   class: Volume
                        y_size: 1
                        x_size: 1
                        z_size: 0
                        actions:
                        -   class: ModifyBlock
                    -   class: Delay
                    -   class: Volume
                        y_size: 2
                        x_size: 2
                        z_size: 0
                        actions:
                        -   class: ModifyBlock
                    -   class: Delay
                    -   class: Volume
                        y_size: 3
                        x_size: 2
                        z_size: 1
                        actions:
                        -   class: ModifyBlock
    parameters:
        destructible: destructible_replace
        range: 12
        undo: 16000
        delay: 250
        axis: z
        orient: true

I will try these out when I can- in theory the undo system is supposed to catch this, but it's very tricky so something may be going wrong there.

Hey Nathan,
Did you have time to test this out already?
If so, did it drop the item frames and its content and do you know a fix for it?

I tried them out just now. I tested both of your earth moves, as well as the builtin Blob spell.

Unfortunately I'm not seeing the problem you're having, though I did find a separate bug. The item frames would sometimes disappear, but they don't drop. The bug is that they don't come back either, when they should be restored as the spell undoes.

So, that leaves me to think if item frames are dropping for you there may be a conflict with some other plugin. Do you have maybe have WorldGuard regions or something else that would lead me to the problem?

I don't think WorldGuard/Towny are involved here, as it happens both inside and outside of protected regions.

It seems like it is caused by ModifyBlockAction modifying the block an item frame (entity, air block) is inside of to something solid.
From my brief (and probably uninformed) look at the codebase it appears that item frame drops are only added to the undo queue when their attached face is modified.

public void onHangingBreak(HangingBreakEvent event) {
// ..
  final BlockFace attachedFace = entity.getAttachedFace();
  Location location = entity.getLocation();
  location = location.getBlock().getRelative(attachedFace).getLocation();
  UndoList undoList = controller.getPendingUndo(location);
  if (undoList != null) {
    event.setCancelled(true);
    undoList.damage(entity);
  }
// ...

Unless of course it is already added to the undo list when ModifyBlockAction is called.
In that action I can only find context.registerForUndo(block); which only operates on the block data, and context.clearAttachables(block); which is probably not called, since this is not an erase brush (right?) and only operates on blocks/title entities.

In the case that I am missing where this case is handled exactly, please let me know. If not, I might be able to propose a patch (though I am not super-familiar with all of the kinks of the undo system).

You may be right, but I'd first like to know why I can't reproduce the issue.

The primary thing reported that I think should be covered is the item frames themselves dropping. That's generally handled by the item spawn event and a lot of annoying checks to try to decide if this item spawning was due to a block modified by a spell. (I really wish there was just an event for an item spawning due to a block breaking, or some kind of spawn reason, or something. Oh, well).

Were you able to reproduce the item frames actually dropping? If so, I'd love to know how.

Your analysis may be correct for the problem I was able to reproduce, which is that the item frame gets removed and does not come back. Could be a check is needed for the occupied block having been modified, as well as the attached block.

Thank you for looking into this!

You tested with 1.12 as well I presume? Just to rule out behavioral changes.
I can reproduce it in our testing environment without any problems.
(On a freshly compiled magic version)

I did, yes.

I test with this exact config:
https://youtu.be/OWGhssRLuXk

test_cage:
    icon_url: http://textures.minecraft.net/texture/77c999f4dfd915fc5f1d66120334796687c48189124e6ca944fba1aebd986b
    icon: skull_item:4

    actions:
        cast:
        -   class: CustomProjectile
            actions:
            -   class: ChangeContext
                source_at_target: true
                actions:
                -   class: CheckBlock
                    allowed: earth_full_block
                    actions:
                    -   class: Brush
                        sample: true
                        actions:
                        -   class: Sphere
                            actions:
                            -   class: ModifyBlock
                    -   class: Stop
                -   class: Brush
                    brush: stone
                    actions:
                    -   class: Sphere
                        actions:
                        -   class: ModifyBlock
                    -   class: Stop
    parameters:
        range: 16
        target: LivingEntity
        destructible: destructible_replace
        transparent: transparent_no_water
        radius: 5
        thickness: 1
        velocity: 16
        reorient: true
        undo: 4000

I really hope this is not something seemingly unrelated in one of our plugins.
Perhaps it is also worth mentioning that we're using paper.

That would be great!

Thanks, I will try again!

Maybe my test was invalid, it could even be something as strange as depending on from which direction you can cast the spell, maybe the way I was testing the attached block got touched before the block the item frame occupies, so it was covered?

You may be right that simply adding a check for the occupied block in HangingController could fix it. I'll take a look when I can.

Can you please test latest, or this commit:

0a54faa

I tried with your test_cage spell from a variety of angles and this seems to fix the problem, the item frames no longer drop or disappear. I used the same spacing as in your video. Let me know if you still see an issue, please!

Works like a charm!
No items are dropped and the item frames are successfully restored.

Thank you so much for the help.

Any idea why this only now causing problems btw?

That is an excellent question, honestly I’m not sure. It’s possible I broke it a while back trying to fix some other undo bug, but I haven’t touched it much recently.