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

[Feature] add advanced place method to automata core when compass peripheral is equipped #593

Merged
merged 17 commits into from
May 10, 2024
Merged

[Feature] add advanced place method to automata core when compass peripheral is equipped #593

merged 17 commits into from
May 10, 2024

Conversation

zyxkad
Copy link
Collaborator

@zyxkad zyxkad commented Apr 25, 2024 •
edited
Loading

PLEASE READ THE GUIDELINES BEFORE MAKING A CONTRIBUTION

  • Please check if the PR fulfills these requirements
  • The commit message are well described
  • Docs have been added / updated (for features or maybe bugs which were noted). If not, please update the needed documentation here. This is not mandatory
  • All changes have fully been tested

  • What kind of change does this PR introduce? (Bug fix, feature, ...)
    Feature & Bug fix

  • What is the current behavior? (You can also link to an open issue here)
    N/A
    Also going to fix

  • What is the new behavior (if this is a feature change)?
    Now the compass turtle can place blocks around the turtle with specific facing, which makes 3D printer easier.

  • Does this PR introduce a breaking change? (What changes might users need to make in their scripts due to this PR?)
    No

  • Other information:

@zyxkad zyxkad changed the title dd advanced place method to the compass peripheral [Feature] add advanced place method to the compass peripheral Apr 25, 2024
@zyxkad zyxkad marked this pull request as ready for review April 25, 2024 04:41
@SirEndii SirEndii self-requested a review April 26, 2024 10:26
@zyxkad zyxkad marked this pull request as draft April 27, 2024 18:27
@zyxkad zyxkad marked this pull request as ready for review April 27, 2024 23:17
@zyxkad zyxkad linked an issue Apr 27, 2024 that may be closed by this pull request
Impossible to craft compass turtle #599
Closed
@zyxkad zyxkad added this to the 0.7.x milestone Apr 27, 2024
@SirEndii
Copy link
Member

I am not 100% if I like this. This adds world manipulation functions to the Compass Turtle, whereas the Meta Turtles should actually be used for this
I know that the meta turtles currently don't have a place function
Would it be a better idea to add the accurate place feature to the meta turtles IF the compass is attached? The main problem I see with that, that it would use one peripheral slot for these two added functions

The implementation of these features looks good, I don't see anything in it, I would change

@zyxkad
Copy link
Collaborator Author

zyxkad commented Apr 29, 2024

Would it be a better idea to add the accurate place feature to the meta turtles IF the compass is attached?

Good point, I'll try that

@zyxkad
Copy link
Collaborator Author

zyxkad commented Apr 29, 2024

However, can you review #597 first then? because merge two branch that changed same thing will be horrible

@zyxkad zyxkad marked this pull request as draft April 30, 2024 20:27
@zyxkad zyxkad changed the title [Feature] add advanced place method to the compass peripheral [Feature] add advanced place method to automata core when compass peripheral is equipped Apr 30, 2024
@zyxkad zyxkad marked this pull request as ready for review April 30, 2024 21:41
@zyxkad
Copy link
Collaborator Author

zyxkad commented Apr 30, 2024

Would it be a better idea to add the accurate place feature to the meta turtles IF the compass is attached? The main problem I see with that, that it would use one peripheral slot for these two added functions

Done

@SirEndii
Copy link
Member

I see some commits here
Is this ready for review or a draft?

@zyxkad
Copy link
Collaborator Author

zyxkad commented Apr 30, 2024

Yeah I just added some comment, it's ready for review

@zyxkad
Copy link
Collaborator Author

zyxkad commented Apr 30, 2024

well I just noticed that forward is a verb, I should use front instead

@zyxkad
Copy link
Collaborator Author

zyxkad commented May 1, 2024

Ready for review again

SirEndii
SirEndii approved these changes May 10, 2024
View reviewed changes
src/main/java/de/srendi/advancedperipherals/common/configuration/UnsafeConfig.java

public static void build(final ForgeConfigSpec.Builder builder) {
enableUnsafe = builder.comment("By setting this value to true, I understand all operations below are danger to my adventure, and if they caused unexpected behaviour in my world, I will not consider it as AP's liability").define("enableUnsafe", false);
ignoreTurtlePeripheralItemNBT = builder.comment("Ignore turtle peripheral item's NBT when equipping. **YOU WILL LOST ALL NBT ON THE ITEM**").define("ignoreTurtlePeripheralItemNBT", false);
Copy link
Member

Choose a reason for hiding this comment

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

Typos. lost -> lose, behaviour -> behavior (American English)

Copy link
Member

Choose a reason for hiding this comment

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

I requested this change, forgot that I requested it, then I merged the PR 😮‍💨

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol

@SirEndii SirEndii merged commit 6b299b5 into IntelligenceModding:dev/1.19.2 May 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SirEndii SirEndii SirEndii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Advanced Peripherals
Status: Done
Milestone
0.7.x
Development

Successfully merging this pull request may close these issues.

Impossible to craft compass turtle
2 participants
@zyxkad @SirEndii