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 clipboard support to //deform #2276

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add clipboard support to //deform #2276

wants to merge 8 commits into from

Conversation

TomyLobo
Copy link
Collaborator

@TomyLobo TomyLobo commented Mar 8, 2023

This adds a -l flag to //deform and //brush deform which, when used, will fetch blocks from the clipboard instead of the world.
Additionally, coordinate transformations that would normally be based on the selection position+size are based on the clipboard's position+size when using that mode.

Example usage:
image
The shape on the right is the result of copying the square shape on the left and deforming the selection with the command on screen.

image
This is the following brush using the same clipboard contents as before:
//brush deform -l sphere 6 x=-x;y=0;z=-z

Addresses parts of #1554

@TomyLobo TomyLobo force-pushed the deform-clipboard branch 2 times, most recently from d7499d1 to 7077c74 Compare March 8, 2023 04:35
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #2276 (6116518) into master (acc4b59) will increase coverage by 0.02%.
The diff coverage is 6.93%.

❗ Current head 6116518 differs from pull request most recent head 7f88a3e. Consider uploading reports for the commit 7f88a3e to get more accurate results

@@             Coverage Diff             @@
##             master   #2276      +/-   ##
===========================================
+ Coverage      8.68%   8.71%   +0.02%     
- Complexity     1041    1044       +3     
===========================================
  Files           881     883       +2     
  Lines         46291   46370      +79     
  Branches       5160    5158       -2     
===========================================
+ Hits           4022    4042      +20     
- Misses        42087   42144      +57     
- Partials        182     184       +2     
Impacted Files Coverage Δ
...src/main/java/com/sk89q/worldedit/EditSession.java 0.77% <0.00%> (-0.01%) ⬇️
...rc/main/java/com/sk89q/worldedit/LocalSession.java 0.00% <0.00%> (ø)
...ava/com/sk89q/worldedit/command/BrushCommands.java 3.68% <0.00%> (-0.13%) ⬇️
...a/com/sk89q/worldedit/command/GeneralCommands.java 8.67% <0.00%> (-0.28%) ⬇️
...om/sk89q/worldedit/command/GenerationCommands.java 3.30% <0.00%> (ø)
...va/com/sk89q/worldedit/command/RegionCommands.java 1.20% <0.00%> (-0.05%) ⬇️
...a/com/sk89q/worldedit/function/factory/Deform.java 0.00% <0.00%> (ø)
...k89q/worldedit/math/transform/SimpleTransform.java 0.00% <0.00%> (ø)
.../regions/shape/WorldEditExpressionEnvironment.java 0.00% <0.00%> (ø)
...in/java/com/sk89q/worldedit/session/Placement.java 0.00% <0.00%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TomyLobo
Copy link
Collaborator Author

rebased

@TomyLobo TomyLobo force-pushed the deform-clipboard branch 2 times, most recently from 39257c9 to bb880d3 Compare March 23, 2023 02:43
@TomyLobo
Copy link
Collaborator Author

rebased this on top of most of the //placement PR in order to be able to use the new Placement system for the Deform brush.
Also marked as draft because the Current code is a WIP

@TomyLobo TomyLobo marked this pull request as draft March 23, 2023 02:45
@TomyLobo TomyLobo force-pushed the deform-clipboard branch 2 times, most recently from 83eb6aa to 7f88a3e Compare April 5, 2023 04:54
@TomyLobo
Copy link
Collaborator Author

Moved the (contentious and not yet rebased) //brush deform changes into a separate deform-brush-clipboard branch and rebased again.
I'll unmark this as a draft once I'm done testing if it still works.

I might rebase deform-brush-clipboard later, but that's somewhat of a failed implementation and probably should be redone from scratch anyway.

@TomyLobo TomyLobo changed the title Add clipboard support to //deform and //brush deform Add clipboard support to //deform Jul 14, 2024
@TomyLobo TomyLobo marked this pull request as ready for review July 14, 2024 16:41
@TomyLobo TomyLobo requested a review from a team as a code owner July 14, 2024 16:41
@TomyLobo
Copy link
Collaborator Author

Checks are passing, deform-brush-clipboard pushed
deform-brush-clipboard is untested.
deform-clipboard (this PR) is tested and working well, except for one issue:
for flat clipboards (i.e. clipboards which are only 1 thick on any axis) non-flat selections map some points outside the clipboard.
A workaround for the user is to set the flat axis to 0.

Two alternatives were considered:
option B: adjust only when inverting
advantage: //deform -l 1 fills the entire selection even if the clipboard only is 1 thick
disadvantage(?): any coordinate on the axis in which the clipboard only is 1 thick will yield the same raw coordinate, meaning it's impossible to go outside the box on that axis anymore, with or without -l
disadvantage: the transform's implied matrix is singular if the clipboard only is 1 thick
disadvantage: the resulting transform is technically no longer really its own inverse (though transform.inverse().inverse() == transform still holds true)

option C: add 0.5 to the region/clipboard's min/max
advantage: no singularities, continuous, no special cases
disadvantage: breaks people's existing expressions for //generate, //generatebiome, //deform and //brush deform
disadvantage: makes the relationship between block coordinates and -1..1 coordinates more complex
disadvantage: coordinates never reach -1 or 1, breaking expressions that rely on that

@TomyLobo
Copy link
Collaborator Author

Now that #2582 is merged, I rebased and force-pushed this and #2580
Will test now, but I don't expect any issues, unless the heat is getting to me more than I thought :)

@TomyLobo TomyLobo enabled auto-merge July 21, 2024 07:55
@TomyLobo
Copy link
Collaborator Author

I'm done testing and enabled auto-merge.
So once someone reviews this, it's in :)

Copy link
Member

@me4502 me4502 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd prefer if someone else reviewed this too as I'm less familiar with the expression/deform system

@TomyLobo
Copy link
Collaborator Author

just force-pushed to change a javadoc description from input/output to source/target

@TomyLobo
Copy link
Collaborator Author

force-pushed to rename SimpleTransform to ScaleAndTranslateTransform, as suggested by @wizjany.

@TomyLobo
Copy link
Collaborator Author

TomyLobo commented Aug 19, 2024

I just rebased this again.
Any objections to merging it once it's done building?

@octylFractal
Copy link
Member

I'd like to give it a review, but I can't immediately because I'm working.

@octylFractal octylFractal self-requested a review August 19, 2024 18:35
@TomyLobo
Copy link
Collaborator Author

TomyLobo commented Sep 9, 2024

this is still done and just pending a review :)

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.

4 participants