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

Error when multiple event-values are present for a default expression #5769

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Jun 27, 2023

Description

Error when multiple event-values are present for a default expression.

This has been a common issue, where Skript silently selects the first event-value when grabbing a default expression.
For my example:

on throwing of an egg:
	send "test"

In theory Skript shouldn't know whether you want to message the egg or player in the send effect, they're both entities.

Currently Skript will send the message to whichever event-value was registered first, or placed in order of subclass. The highest subclass is always first (Skript is usually smart enough to search with converters and get the correct one just not when there is two of same type), so this will pick the egg. This is why Skript has excluded events for the event-value register method, to manually explain per event-value.

This pull request will do that automatically, as we cannot always manually adjust the error or determine when and where they collide. Also addons.

So for my example above, this will be the following error:
Untitled

Notes:

  • For context if you don't know, in the EffMessage a default expression would be the [%commandsenders%] and this means that the user doesn't need to define the command senders, it'll select from the event-values or any other default expression present. Example: on player join: -> send "Hello!"
  • Unfortunately I had to modify the main event-value getter collection method to be a List return so that we can properly calculate the possible getters. Makes it look harder to understand, but it's what's needed.
  • I changed the command sender noun type name to command sender¦s rather than player¦s/console as a command sender can now be an entity and this avoids a confusing line for EffMessage (The most likely origin of this error).
  • I believe this is a better solution than Event value priority #4858
  • I also disallowed exact duplicate event-value registering, we don't need an addon registering an already present exact event-value. This can be problematic which was an original issue in Event value priority #4858

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

@TheLimeGlass TheLimeGlass added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. feature Pull request adding a new feature. labels Jun 27, 2023
@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Jun 28, 2023

This pull request just helped me solve a Skript user's issue where

on entity target:
    if entity's name is "&6Corrupted Golem":
        if target is not player:
            cancel event

Was not working and the user didn't know why. The reasoning was because target could have been player's target or the entities target and also player is an assumption so a would be needed to describe the type of the target. Solution code would be:

on entity target:
    if entity's name is "&6Corrupted Golem":
        if target of event-entity is not a player:
            cancel event

Crazy how I luckily still had this pull request on my current branch as I tested their code to help them out.
Forced me to properly understand what Skript was reading and understand the problem with the new given error.

@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 17, 2023 08:03
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

nice pr!

src/main/resources/lang/default.lang Show resolved Hide resolved
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Sep 20, 2023
@TheLimeGlass TheLimeGlass merged commit 040cb85 into dev/feature Sep 26, 2023
@TheLimeGlass TheLimeGlass deleted the feature/default-expression-multiple branch September 26, 2023 06:57
@TheLimeGlass TheLimeGlass removed the request for review from sovdeeth October 15, 2023 01:31
@sovdeeth sovdeeth added the breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) label Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. feature Pull request adding a new feature. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants