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

Add a RunsAfter annotation to allow ordering of rasterizers #3825

Merged
merged 5 commits into from Jan 20, 2020

Conversation

@askneller
Copy link
Contributor

askneller commented Jan 16, 2020

Contains

Adds a RunsAfter annotation that indicates one rasterizer must be run after another.

How to test

Add the annotation to a rasterizer. E.g.

@RegisterPlugin
@RunsAfter(CaveRasterizer.class)
public class SomeRasterizer implements WorldRasterizerPlugin {

During world generation you can place a breakpoint at WorldBuilder line 108. When it hits the breakpoint inspect the orderedRasterizers list and confirm that SomeRasterizer is after CaveRasterizer.

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jan 16, 2020

Hooray Jenkins reported success with all tests good!

@askneller askneller changed the title Add a RunsAfter abbotation to allow ordering of rasterizers Add a RunsAfter annotation to allow ordering of rasterizers Jan 16, 2020
@Qwertygiy

This comment has been minimized.

Copy link
Contributor

Qwertygiy commented Jan 16, 2020

Looks good to me. There is one minor thing; I feel like the name should imply that the annotated rasterizer requires the other rasterizer to be run before it can be run, much as the Requires annotation exists for facets.

This is a good solution for situations similar to that, where the rasterizers could not logically be run without the presence of their predecessor, like the CaveFloor generation not doing anyone a lick of good without a Cave existing! But the name RunsAfter could suggest that there's no dependency involved and that it's merely for ordering purposes, which it shouldn't be.

@askneller

This comment has been minimized.

Copy link
Contributor Author

askneller commented Jan 17, 2020

Good point. How about I change it to something like RequiresRasterizer so that it doesn't conflict with the existing Requires?

@Qwertygiy

This comment has been minimized.

Copy link
Contributor

Qwertygiy commented Jan 17, 2020

Sounds perfect to me!

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jan 18, 2020

Hooray Jenkins reported success with all tests good!

@Qwertygiy Qwertygiy merged commit d287ca1 into MovingBlocks:develop Jan 20, 2020
2 checks passed
2 checks passed
LGTM analysis: Java No new or fixed alerts
Details
default Build finished.
Details
@Cervator Cervator added this to the v2.3.0 milestone Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.