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

Track direct chunk access #3855

Merged
merged 1 commit into from
May 5, 2023

Conversation

Yeregorix
Copy link
Member

@Yeregorix Yeregorix commented Apr 29, 2023

Track block changes made with direct calls to LevelChunk#setBlockState.
Fixes the third point in #3811.

I made some tests with worldedit-forge and a small test plugin I wrote:

@Listener
public void onBlockChange(ChangeBlockEvent.All e, @First ServerPlayer p) {
	Sponge.server().sendMessage(Component.text(e.transactions().size() + " block changes by ").append(TestUtil.debugStack(e.cause())));
	if (e.transactions().size() % 2 == 0) {
		Sponge.server().sendMessage(Component.text("CANCELLED"));
		e.setCancelled(true);
	}
}

The event is fired correctly and all transactions are grouped into a single event with the worldedit command being the root cause. Cancelling the event, properly cancels block changes.

To have an idea of the performance impact, I selected a 100x100x100 area (1 million blocks) and I measured the average time for command //set 33%granite,33%stone,33%andesite to execute on my computer.
With Sponge: ~5.5s
Without Sponge: ~3.2s

It is a very imprecise benchmark but it shows the order of magnitude of the tracking overhead.
The performance impact is significant but IMO tolerable for now.

@Yeregorix Yeregorix requested a review from gabizou May 2, 2023 18:39
Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

Looks fine. I did intend to have logged warnings in the other PR so that there's some visibility into possible overhead with certain cases. There's also the possibility of simply enabling WorldEdit to throw the events itself and call the direct chunk setter without Sponge's tracking by itself.

But that may blow up the scope of the PR for now.

@Yeregorix
Copy link
Member Author

I think I will just merge this PR as it is. Warnings can be added in a new PR.

As for worldedit-forge throwing the events, that would require depending on Sponge. At this point, they could just use the API like worldedit-sponge does. I think worldedit-forge should stay forge only. People looking for perfect compatibility should use worldedit-sponge (when it will be fixed).

@Yeregorix Yeregorix merged commit e128015 into SpongePowered:api-8 May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants