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

Using 'Contains' for List Variables #2393

Closed
AsuDev2 opened this issue Aug 22, 2019 · 5 comments
Closed

Using 'Contains' for List Variables #2393

AsuDev2 opened this issue Aug 22, 2019 · 5 comments
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. completed The issue has been fully resolved and the change will be in the next Skript update. priority: high Issues with potentially high impact that could be harmful to users.

Comments

@AsuDev2
Copy link

AsuDev2 commented Aug 22, 2019

Tried on Version 1.13.2/1.14 using Skript 2.4-beta5

Recently I ran a stress test comparing looping list variables to find an object and also using contains to find an object. It seems that looping is actually WAY faster. I don't know why its the case but I was wondering if there could be an enhancement for using 'Contains' with list variables so that its as fast as looping.

The Stress Test:
3000 random strings with 12 random chars each in a list variable
set var to random string out of the list variable

Looping to find the random string:
Looping Speed: Took 3ms on average (2ms-4ms) to find the random object.

Using contains to find the random string:
Contains Speed: Took 600ms on average (500ms-700ms) to find the random object.

@Blueyescat
Copy link
Contributor

Blueyescat commented Aug 22, 2019

simple code to test

function listContains(list: objects, o: object) :: boolean:
    loop {_list::*}:
        if loop-value is {_o}:
            return true
    return false

on load:
    set {x::*} to integers between 1 and 1000
    
    set {_s} to (unix timestamp of now * 1000)
    if {x::*} contains 500:
        broadcast "contains - %(unix timestamp of now * 1000) - {_s}%ms"

    set {_s} to (unix timestamp of now * 1000)
    if listContains({x::*}, 500) is true:
        broadcast "listContains - %(unix timestamp of now * 1000) - {_s}%ms"

@FranKusmiruk
Copy link
Member

@Blueyescat May I ask, what's the output effect you're using there? If it is a custom syntax, you may as well provide it or just change that to a broadcast.

@TheBentoBox TheBentoBox added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. priority: high Issues with potentially high impact that could be harmful to users. labels Aug 26, 2019
@TheBentoBox
Copy link
Member

TheBentoBox commented Aug 26, 2019

The contains effect has long been problematic. It's slow and historically has simply not even always worked properly.

In this specific scenario (checking contains on a list of objects) the operation the condition is doing has no reason to be 200x slower than the same condition being rewritten as a Skript function. There is clearly some large for optimization and fixes here and given how often this condition is used it should be addressed sooner rather than later IMO.

@Malfrador
Copy link

Malfrador commented Aug 26, 2019

Also, looking for variables (specifically locations) does not seem to work using "contains"

command /capture:
	trigger:
		set {_loc} to location of block below player
		set {_rg} to "test3"
		set {_p} to player
		broadcast "Loop:"
		loop {CPs.%{_rg}%::*}:
			broadcast "%loop-value%"
		broadcast "Loc: %{_loc}%"
		# This does not work. Broadcast loop-value prints {_loc} in the list, but "contains" does not find it. 
		if {CPs.%{_rg}%)::*} contains {_loc}:
			broadcast "test"

{_loc} is even in there twice, still does not find it and never shows "test"
Chat:

javaw_2019-08-26_18-01-44

Looping works fine:

	loop {CPs.%{_rg}%::*}:
		if loop-value is {_loc}:
			broadcast "Yeah!"

@bluelhf
Copy link
Contributor

bluelhf commented Aug 31, 2019

Also, looking for variables (specifically locations) does not seem to work using "contains"

command /capture:
	trigger:
		set {_loc} to location of block below player
		set {_rg} to "test3"
		set {_p} to player
		broadcast "Loop:"
		loop {CPs.%{_rg}%::*}:
			broadcast "%loop-value%"
		broadcast "Loc: %{_loc}%"
		# This does not work. Broadcast loop-value prints {_loc} in the list, but "contains" does not find it. 
		if {CPs.%{_rg}%)::*} contains {_loc}:
			broadcast "test"

{_loc} is even in there twice, still does not find it and never shows "test"
Chat:

javaw_2019-08-26_18-01-44

Looping works fine:

	loop {CPs.%{_rg}%::*}:
		if loop-value is {_loc}:
			broadcast "Yeah!"

Cannot replicate with following code:

function lookup(l: locations, p: player):
  log "looking for %{_p}%'s location (%{_p}'s location%) in this list:"
  loop {_l::*}:
    if loop-value isn't {_p}'s location:
      log "  %loop-value%"
    else:
      log "  %loop-value% <--- FOUND"
  if {_l::*} contains {_p}'s location:
    log "found"
  else:
    log "not found"

Output for !lookup((spawn of me's world, (location(0,0,0,world of me))), me):

looking for blce's location (x: 10008.18, y: 85, z: 10001.32) in this list:
x: -1, y: 63, z: 0
x: 0, y: 0, z: 0
not found

Output for !lookup((spawn of me's world, (location(0,0,0,world of me)), location of me), me):

looking for blce's location (x: 10008.18, y: 85, z: 10001.32) in this list:
x: -1, y: 63, z: 0
x: 0, y: 0, z: 0
x: 10008.18, y: 85, z: 10001.32 <--- FOUND
found

I think this might be your issue:
image

@bensku bensku added the completed The issue has been fully resolved and the change will be in the next Skript update. label Sep 1, 2019
@bensku bensku closed this as completed Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. completed The issue has been fully resolved and the change will be in the next Skript update. priority: high Issues with potentially high impact that could be harmful to users.
Projects
None yet
Development

No branches or pull requests

7 participants