-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support for building selected litematic #4178
Open
thojo0
wants to merge
3
commits into
cabaletta:1.19.4
Choose a base branch
from
thojo0:1.19.4
base: 1.19.4
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+42
−5
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baritone.getBuilderProcess().buildOpenLitematic(LitematicaHelper.getSelectedIndex());
this would remove the need of a second buildOpenLitematic method in BuilderProcess however it also wouldnt be wrong to remove the manual selection entirely and just keep the one that builds the schematic litematica has selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jeah that's right, i added the second one with the idea that you can easily use it through the api, but of course you can just send the command directly to the execute function, what works better anyways often.
generally adding options i think is good but for intuition maybe only using the selected one makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about building placements by name? It's intuitive and with proper tab completion is't easy to use.
For easier API usage I think the best option would be a method taking the desired
SchematicPlacement
(Why are we even using indices in the first place?). AddingbuildOpenLitematic()
is good for consistency withbuildOpenSchematic()
though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
names wont work well because you can have multiple schematics with the same name open. how would you decide which schematic is meant if you have 2 "myHouse.litematic"
as to why integers are used instead of SchematicPlacements thats because SchematicPlacement is a class from litematica and i didnt want to add a dependency to a optional mod to the BuilderProcess this way the only Class with a direct dependency on litematica should be LitematicaHelper and methods of that class should only be called after the LitematicaHelper.isLitematicaPresent() method returned true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added an branch with only the selected placement thojo0/baritone/only_selected_litematic
i will add a pull request if requested for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are talking about placements here and I though litematica enforces unique names on those, like it does for selection and subregion names. But apparently you can have indistinguishable placements. Reporting an error for duplicates would be an option, though it's certainly not as nice anymore.