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.

Sign up for GitHub

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

Jump to bottom

feat: allow placement of minecraft gen features and structures #2364

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: allow placement of minecraft gen features and structures #2364

wants to merge 15 commits into from
+3,337 −821

Conversation

dordsor21
Copy link
Member

@dordsor21 dordsor21 commented Jul 17, 2023 •
edited
Loading

@dordsor21 dordsor21 requested a review from a team as a code owner July 17, 2023 21:52
@github-actions github-actions bot added the Feature This PR adds a new feature label Jul 17, 2023
@SirYwell
Copy link
Member

This only works if the EditSession is backed by an actual World, I think this should be outlined in the Javadocs.

@dordsor21
Copy link
Member Author

noticed upstream has this functionality, will change to match them and cherry-pick where appropriate

@dordsor21 dordsor21 marked this pull request as draft October 14, 2023 19:35
@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 50a3328 to 594527f Compare October 24, 2023 16:33
@dordsor21 dordsor21 marked this pull request as ready for review October 24, 2023 16:33
@dordsor21 dordsor21 changed the title feat: allow placement of minecraft ConfiguredFeatures feat: allow placement of minecraft gen features and structures Oct 24, 2023
@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

SirYwell
SirYwell requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

As there's a mix of structures and features here, what exactly do we add what WE doesn't add? StructureType is in the WE source here, but the commands and the tools for it are in the FAWE source, that's a bit confusing to me right now.

...ain/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/PaperweightFaweAdapter.java Outdated
@@ -555,6 +571,164 @@ protected ServerLevel getServerLevel(final World world) {
return ((CraftWorld) world).getHandle();
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

There is some similarity to the generateTree code here. Can we move it to the FaweAdapter too? I also think the Map<BlockPos, CraftBlockState> can be simplified to a list similar to how it's done in generateTree now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to look at doing so yeah. It would be more complicated though as the method changes per version a couple of times

Copy link
Member Author

@dordsor21 dordsor21 Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I don't think that's very viable. We're not able to make any meaningful amount of abstraction as almost every line has a NMS class

...om/sk89q/worldedit/bukkit/adapter/ext/fawe/v1_20_R2/PaperweightServerLevelDelegateProxy.java Outdated
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

public class PaperweightServerLevelDelegateProxy implements InvocationHandler {
Copy link
Member

Choose a reason for hiding this comment

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

This class seems to be unused in that version. Did you miss upstream changes in the PaperweightAdapter? Generally, do we use that code (the changes in this PR to files in ext/) ourselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think there's some cleanup required to marry the implementation across each of the adapter versions

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, no, we don't use the changes in the ext classes, however, I think it's useful to have the WE code there for reference

@dordsor21
Copy link
Member Author

dordsor21 commented Nov 23, 2023 •
edited
Loading

As there's a mix of structures and features here, what exactly do we add what WE doesn't add? StructureType is in the WE source here, but the commands and the tools for it are in the FAWE source, that's a bit confusing to me right now.

The various Tool/Factory classes are in FAWE because they are classes added by FAWE. The actual command methods are in com.sk89q because all commands are there, and are effectively just class edits, not new classes.

This implementation was in line with how we handle tree generation, and if we can avoid using an InvocationHandler when it's not needed I think we should (the same for any reflection really)

@dordsor21 dordsor21 requested a review from SirYwell December 3, 2023 15:13
@NotMyFault
Copy link
Member

Please rebase for 1.20.3/4

@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

SirYwell
SirYwell requested changes Mar 28, 2024
View reviewed changes
worldedit-core/src/main/java/com/fastasyncworldedit/core/command/tool/FeaturePlacer.java
boolean successful = false;

final BlockVector3 pos = clicked.toVector().add().toBlockPoint();
for (int i = 0; i < 10; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the repetition for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it exists/existed somewhere in Minecaaft or upstream? There is often a random aspect to a feature or structure (e.g. tree height) that means it might fail to be constructed occasionally. This is an arbitrary way to try a few times basically

.../main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_18_R2/PaperweightFaweAdapter.java Outdated Show resolved Hide resolved
@SirYwell
Copy link
Member

For some reason there is a folder with a . instead of nested folders for the 1.20.4 adapter
dot

@SirYwell
Copy link
Member

From a quick test, it looks like e.g. //structure bastion_remnant throws an exception. I don't know if we can fix it, but I think it should at least be handled in a way that doesn't spam the console making it look like FAWE is doing something wrong.

Similarly, features and structures often cannot be placed with messages "Failed to generated structure. Is it a valid spot for it?" and "This feature cannot go here. Ensure the area meets the requirements." In fact, I wasn't able to spawn any feature for now. Can we somehow lift those restrictions? Otherwise, this is very limiting in its use.

@dordsor21
Copy link
Member Author

From a quick test, it looks like e.g. //structure bastion_remnant throws an exception. I don't know if we can fix it, but I think it should at least be handled in a way that doesn't spam the console making it look like FAWE is doing something wrong.

Yes probably worth doing that

Similarly, features and structures often cannot be placed with messages "Failed to generated structure. Is it a valid spot for it?" and "This feature cannot go here. Ensure the area meets the requirements." In fact, I wasn't able to spawn any feature for now. Can we somehow lift those restrictions? Otherwise, this is very limiting in its use.

I tried, but no. Minecraft generation code is complete spaghetti and it would require an incredible amount of reflection and likely need us to perform changes to the byte code. The command in upstream WE has the same limitations

@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 7, 2024

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 9f3f70d to 0a3a607 Compare September 26, 2024 18:07
@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 0a3a607 to 2807219 Compare October 26, 2024 09:54
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2024

Please take a moment and address the merge conflicts of your pull request. Thanks!

@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 2807219 to 0773b89 Compare November 4, 2024 17:49
@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 0773b89 to 535c5e9 Compare November 16, 2024 11:59
@github-actions GitHub Actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

me4502 and others added 14 commits December 24, 2024 17:52
@me4502 @dordsor21
Add a feature generator and allow undoing of feature placement (Engin…
f81fcae 
…eHub/WorldEdit#2239)

* Add a feature generator and allow undoing of feature placement [WIP]

Apply changes to Forge as well

Use proper translatable components

* Add a brush version of the feature command

Use Java proxy classes

* Add for Bukkit (only 1.19.3 for now)

Clean up the proxies to use a switch

Checkstyle is grumpy

Add the obfuscated versions

Remove debug text

Fix missed "destroyBlock" deobfuscated proxy function

* checkstyle
@me4502 @dordsor21
Add structure generator command
25d7ca7 
(cherry picked from commit 5ca80395a35100c2d7686ad8839baaa57f8db07c)
@dordsor21
FAWE-inate
a70cfdf
@dordsor21
cleanup
0034f38
@dordsor21
bring more in line with upstream
07188a1
@dordsor21
Update 1.20.3/4 adapters
aad13f5
@dordsor21
fix ext.fawe -> ext/fawe
f7387ad
@dordsor21
Address vector deprecations
c4ccee4
@dordsor21
Upstream changes, use correct list of cached block changes
93f3573
@dordsor21
Undo
865456a 
Minecraft has a lot of different methods between private and public to determine if structures can be placed. We cannot possibly cover all of them whilst also ensuring issues do not arise with generic "true"s
@dordsor21
Cleanup
44dd486
@dordsor21
Cleanup after merge
9024018
@dordsor21
Cleanup and add for 1.21
c5636af
@dordsor21
Add for 1.21.3
5face17
@dordsor21 dordsor21 force-pushed the feat/feature-placement branch from 535c5e9 to 5face17 Compare December 26, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SirYwell SirYwell Awaiting requested review from SirYwell

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Feature This PR adds a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generation of Minecraft features and structures
4 participants
@dordsor21 @SirYwell @NotMyFault @me4502