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

Adds CondHasLineOfSight #5562

Merged
merged 12 commits into from Jul 31, 2023
Merged

Adds CondHasLineOfSight #5562

merged 12 commits into from Jul 31, 2023

Conversation

sovdeeth
Copy link
Member

Description

Adds CondHasLineOfSight. Checks if one or more living entities have unobstructed lines of sight to the target entities/locations.


Target Minecraft Versions:
Requirements:
Related Issues: #4185

@sovdeeth sovdeeth changed the title Line of sight Adds CondHasLineOfSight Mar 29, 2023
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali 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 ⚡

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Mar 29, 2023
Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

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

This can be a PropertyCondition

@sovdeeth
Copy link
Member Author

This can be a PropertyCondition

I considered that, but no other PropertyCondition I looked at had a second expression. Since I'd have to override the init anyway, as well as not use the PC register method (because of "has no line of sight"), I thought it'd be better to just use a normal condition.
Do you think it's worthwhile converting it to a property condition?

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Mar 30, 2023

This can be a PropertyCondition

I considered that, but no other PropertyCondition I looked at had a second expression. Since I'd have to override the init anyway, as well as not use the PC register method (because of "has no line of sight"), I thought it'd be better to just use a normal condition. Do you think it's worthwhile converting it to a property condition?

Yes, I don't think it needs no. You can also just keep how it's registered.

@sovdeeth
Copy link
Member Author

This can be a PropertyCondition

I considered that, but no other PropertyCondition I looked at had a second expression. Since I'd have to override the init anyway, as well as not use the PC register method (because of "has no line of sight"), I thought it'd be better to just use a normal condition. Do you think it's worthwhile converting it to a property condition?

Yes, put the no as a parser mark.

Doesn't that allow x doesn't have no line of sight to y?

@sovdeeth
Copy link
Member Author

sovdeeth commented Mar 30, 2023

I'm also not sure how I'd go about getting all the target without access to the event in PropertyCondition#check(T t).

@Ankoki
Copy link
Contributor

Ankoki commented Mar 31, 2023

I don't think this should be a PropertyCondition, as that aims at one expression not two

sovdeeth and others added 2 commits March 31, 2023 11:47
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.

Little nitpick I had

@AyhamAl-Ali AyhamAl-Ali merged commit 21d2420 into SkriptLang:master Jul 31, 2023
4 checks passed
Moderocky pushed a commit to Moderocky/Skript that referenced this pull request Sep 16, 2023
@sovdeeth sovdeeth deleted the LineOfSight branch September 26, 2023 17:14
NotSoDelayed pushed a commit to NotSoDelayed/Skript that referenced this pull request Oct 30, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants