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

EffBlockUpdate #7065

Open
wants to merge 26 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link

@TheAbsolutionism TheAbsolutionism commented Sep 9, 2024

Description

This PR aims to allow getting block states of a block, force updating those states, and force updating blocks as blockdata.

This was ported over from SkBee made by ShaneBee, Credits go to him.
I did however ask for his permission before doing so, in which he gave me his blessing.

I also added something original as a nice touch to it


Target Minecraft Versions: any
Requirements: none
Related Issues: #6394

Added, EffBlockUpdate
Added, ExprBlockState
Added, Blockstate in lang
Added, Blockstate in Classes
Added, Test
@Efnilite
Copy link
Member

Efnilite commented Sep 9, 2024

#7020 (comment)

@TheAbsolutionism
Copy link
Author

TheAbsolutionism commented Sep 9, 2024

#7020 (comment)

Yeah I know, before I even ported it over, I had started making it from scratch without referencing from SkBee. I understand the entire thing and when I got the basic setup done, it was looking almost like SkBee's.

Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Just some styling type stuff, didn't have time to properly go through it at all.

src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffBlockUpdate.java Outdated Show resolved Hide resolved
@sovdeeth sovdeeth added feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question labels Sep 12, 2024
@sovdeeth
Copy link
Member

#7020 (comment)

Yeah I know, before I even ported it over, I had started making it from scratch without referencing from SkBee. I understand the entire thing and when I got the basic setup done, it was looking almost like SkBee's.

I'd also like to double down here and say that even if you wrote it and it looked like skbee's, you should still write it yourself! for one (as pikachu has mentioned previously) it avoids licensing issues related to GPL vs MIT (which we are slowly moving towards), second, it makes sure you know exactly what the code is doing and why, and third, it avoids potentially falling into the same anchoring bias. The description is a bit unclear on whether you wrote this yourself or copy-pasted it, but I would strongly ask that if you did copy paste sections, you undo that and write it yourself. Though at this point I don't know how effective that is given you'd probably just write the same code you are replacing because of said anchoring.

@TheAbsolutionism
Copy link
Author

it makes sure you know exactly what the code is doing and why

I do know what code is doing and why, I even added my own touch to it which should at the very least show that I do

The description is a bit unclear on whether you wrote this yourself or copy-pasted it

That's mainly because I'm just bad at explaining things

@sovdeeth
Copy link
Member

Yes sorry I didn't mean to imply that you don't understand the code, I just mean that writing it yourself guarantees that you do. It's just one aspect of a few that make me wary of directly ported code.

But really, the big concern that can't really be worked around is the licensing issue.

Also, your self-proclaimed explaining skills notwithstanding, can you elaborate on what exactly you changed/added/wrote and what was ported directly?

@TheAbsolutionism
Copy link
Author

Changed:
Descriptions, to the best of my ability
toString()
Examples

Added / Wrote:
New pattern + "mechanic"
Test
Lang entry
registerClass (bare minimum)

Ported:
Examples
Initial pattern
Original var names
The rest of the registerClass (.parse)

@Moderocky
Copy link
Member

I'm not really clear what value BlockState has as an added type.
All I can see is a complicated way to change something without updating the blocks around it (which sounds useful but this seems like a lot of changes for just that)

@TheAbsolutionism
Copy link
Author

I'm not really clear what value BlockState has as an added type. All I can see is a complicated way to change something without updating the blocks around it (which sounds useful but this seems like a lot of changes for just that)

Well, again, I was just introducing BlockStates into Skript, to "future proof" for any possible PRs that would utilize BlockStates. But if you feel, that it should just be added from a PR that is extensive on the utilization and accessibility of BlockStates, then I can remove it from this PR and just rename the PR.

@TheAbsolutionism TheAbsolutionism changed the title BlockStates EffBlockUpdate Sep 20, 2024
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

my review doesn't matter, and I don't remember what my review was for I believe it's related to a comment nothin important

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants