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

Remove Location -> Chunk Converter #6302

Merged

Conversation

NotSoDelayed
Copy link
Contributor

Description

The Location -> Chunk converter was introduced in #5447 to make use of event locations as event chunks, which it may cause inaccurate event chunk results, fixed in #5965, but the converter was not removed prior to that PR.
This PR removes it to prevent any further unexpected behaviour, such as using location parameters in expressions accepting chunks.


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

@AyhamAl-Ali AyhamAl-Ali added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jan 6, 2024
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jan 6, 2024

Is there any information that this solves anything? What are the related issues? Does it fix them? What would be the point of this pull request if it's just speculation?

@NotSoDelayed
Copy link
Contributor Author

Adding converters may break anything in Skript as they are sensitive when it comes to allow other expressions which are not meant to be converted.
This converter was only added to allow event-chunk to work. Now that the proper event values for those are added, why not undo the converter?

@sovdeeth
Copy link
Member

We may want to wait on this until 2.9, though, as it is technically a breaking change, to my knowledge.

all blocks within location(0,0,0)

@Moderocky
Copy link
Member

We may want to wait on this until 2.9, though, as it is technically a breaking change, to my knowledge.

all blocks within location(0,0,0)

If this currently fetches all the blocks in the chunk, which I'm assuming it does from context, this seems like extremely unintended behaviour and I'm very happy to call that a mistake and allow this in the patch version.

@sovdeeth
Copy link
Member

sovdeeth commented Apr 8, 2024

We may want to wait on this until 2.9, though, as it is technically a breaking change, to my knowledge.

all blocks within location(0,0,0)

If this currently fetches all the blocks in the chunk, which I'm assuming it does from context, this seems like extremely unintended behaviour and I'm very happy to call that a mistake and allow this in the patch version.

Good point. I'd agree.

@Moderocky Moderocky merged commit ae7b01a into SkriptLang:dev/patch Apr 8, 2024
4 checks passed
@Moderocky Moderocky added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Apr 8, 2024
sovdeeth pushed a commit to sovdeeth/Skript that referenced this pull request Apr 18, 2024
Remove Location -> Chunk converter

Co-authored-by: Moderocky <admin@moderocky.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants