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

Fix BlockIgniteEvent #7252

Merged
merged 2 commits into from
Dec 26, 2024
Merged

Fix BlockIgniteEvent #7252

merged 2 commits into from
Dec 26, 2024

Conversation

kyoshuske
Copy link
Contributor

@kyoshuske kyoshuske commented Dec 10, 2024

Description

(edited)
event-block in event BlockIgniteEvent (on ignite) should return getBlock() instead of getIgnitingBlock()

issue occurred on:

  • skript: 2.9.2
  • engine: paper-1.21.1-122

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

@Efnilite Efnilite changed the base branch from master to dev/patch December 10, 2024 21:41
@Efnilite Efnilite added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Dec 10, 2024
@Efnilite
Copy link
Member

Efnilite commented Dec 10, 2024

you should add a CI test for this. feel free to ask if you're not sure how to do this in the discord.
also, does this change still make the other event values return the same for other event causes?

@TheAbsolutionism
Copy link
Contributor

I'm a bit curious as to why it was made like that in the first place. The getIgnitingBlock() returns the block that caused the event-block to catch on fire. So obviously, if it was caught on fire by flint and steel it would be null.
If anything, a custom expression should've been made to use the getIgnitingBlock().
Also, there's no reason to have this event value anymore, since the super class BlockEvent has the getBlock()

@TheAbsolutionism
Copy link
Contributor

It was added 7 years ago by LimeGlass, so maybe back then, that's how it used to be? Not sure

@Moderocky Moderocky added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. 2.10 Targeting a 2.10.X version release labels Dec 24, 2024
@Moderocky Moderocky merged commit 3c94063 into SkriptLang:dev/patch Dec 26, 2024
7 checks passed
Moderocky added a commit that referenced this pull request Dec 30, 2024
* Fix the REMOVE changer of variables (#7163)

* Fix the REMOVE changer of variables

* Fix wording

* Fix tests

* Update Variable.java

---------

Co-authored-by: sovdee <[email protected]>

* Fix Improperly Typed Array in ExprXOf (#7268)

* Add boat data check to prevent error. (#7301)

* Fix BlockIgniteEvent  (#7252)

Update BukkitEventValues.java

Co-authored-by: Moderocky <[email protected]>

* Fix expression conversion (#7165)

* Fix expression conversion

* Extract duplicate code into a separate helper method

* improve conversion strategy

* Add .sk to test file

* Simplify conversion usage

We need to use conversion whenever there are multiple return types. If the expression does not accept our supertype, then we can attempt to convert it, which will already handle safety checks for multiple return types

* SimpleExpression: fix possible return type conversion

This fixes SimpleExpression not converting  possible return types that are not contained in the desired types array. For example, if an expression can return a Number or a String, and we want an Expression that is a Number or an World, it will now include converters for String->Number and String->World

* Use safety checks of ConvertedExpression

* Remove incorrect converter remake

* Move logic from SimpleExpression to ConvertedExpression

---------

Co-authored-by: APickledWalrus <[email protected]>
Co-authored-by: Efnilite <[email protected]>
Co-authored-by: Moderocky <[email protected]>

---------

Co-authored-by: _tud <[email protected]>
Co-authored-by: sovdee <[email protected]>
Co-authored-by: Patrick Miller <[email protected]>
Co-authored-by: kyoshuske <[email protected]>
Co-authored-by: Efnilite <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.10 Targeting a 2.10.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 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.

5 participants