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

Inventory effects & expressions are really slow in 1.12 #772

Closed
TrademarkTM opened this issue Aug 5, 2017 · 9 comments
Closed

Inventory effects & expressions are really slow in 1.12 #772

TrademarkTM opened this issue Aug 5, 2017 · 9 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. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation).

Comments

@TrademarkTM
Copy link

Hello again.
I was trying to make a kits script. However, I noticed that giving items is working insanely slowly in the new versions (5 ms to give 5 items). You can even feel that there's a big delay between each item appearing in your inventory

Here's how I am doing it:

command /kit [<text>]:
	permission: itempackage.kit
	permission message: {@P}
	trigger:
		set {_s} to size of {packages::package::%arg 1%::*}
		loop 36 times:
			if {_n} = {_s}:
				stop loop
			slot loop-number - 1 of player = air
			add 1 to {_n}
			set {_slots::%{_n}%} to loop-number - 1
		loop {packages::package::%arg 1%::*}:
			set slot {_slots::%loop-index%} of player to loop-value

Takes 1~1.5 ms to run.

However, simply giving the items is laggy:

command /kit [<text>]:
	permission: itempackage.kit
	permission message: {@P}
	trigger:
		loop {packages::package::%arg 1%::*}:
			give loop-value to player

Takes 5 ms.

Hope you can fix this.
Thanks!

@bensku
Copy link
Member

bensku commented Aug 5, 2017

Hmm, sounds like a possible bug. Could you check what happens if you just give player same item in loop, without using loop-value. Does it cause performance issues?

@bensku bensku added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 5, 2017
@TrademarkTM
Copy link
Author

Made a full test here:

Insanely slow:

command /test0:
	trigger:
		remove all items from player's inventory

[15:02:38 INFO]: [Skript] # /test0
[15:02:38 INFO]: [Skript] # test0 took 12.803814 milliseconds

Reasonable, but slower than manual clearing

command /test1:
	trigger:	
		clear player's inventory

[15:03:24 INFO]: [Skript] # /test1
[15:03:24 INFO]: [Skript] # test1 took 1.709404 milliseconds

I think it's slow for just 4 items

command /test3:
	trigger:
		set {_a::*} to diamond sword, iron sword, iron axe and stone shovel
		loop {_a::*}:
			give loop-value to player

[15:03:54 INFO]: [Skript] # /test3
[15:03:54 INFO]: [Skript] # test3 took 1.852104 milliseconds

Most performance-friendly inventory clearing:

command /test4:
	trigger:
		loop all items in player's inventory:
			clear loop-item

[15:04:27 INFO]: [Skript] # /test4
[15:04:27 INFO]: [Skript] # test4 took 0.475441 milliseconds

0.6 ms for one item.

command /test5:
	trigger:
		set {_i} to diamond sword of sharpness 5, knockback 2 named "TEST" with lore "HEY"
		give {_i} to player

[15:10:58 INFO]: [Skript] # /test5
[15:10:58 INFO]: [Skript] # test5 took 0.645621 milliseconds

Not bad, but could be better I think.

command /test6:
	trigger:
		if player can hold 64 stone:
			broadcast "CAN HOLD STONE"

[15:11:38 INFO]: [Skript] # /test6
[15:11:38 INFO]: CAN HOLD STONE
[15:11:38 INFO]: [Skript] # test6 took 0.533713 milliseconds

Faster than looping, so fine

[15:12:22 INFO]: [Skript] # /test7
[15:12:22 INFO]: HAS DIAMOND SWORD
[15:12:22 INFO]: [Skript] # test7 took 0.455576 milliseconds
command /test8:
	trigger:
		broadcast "%number of diamond sword in player's inventory%"

[15:14:56 INFO]: [Skript] # /test8
[15:14:56 INFO]: 5
[15:14:56 INFO]: [Skript] # test8 took 0.404589 milliseconds
command /test9:
	trigger:
		remove 64 diamond from player's inventory

[15:17:53 INFO]: [Skript] # /test9
[15:17:53 INFO]: [Skript] # test9 took 0.372805 milliseconds

I am comparing only player's tool, I don't think it should take 0.5 ms.

command /test10:
	trigger:
		tool = diamond sword
		broadcast "IS DIAMOND SWORD"

[15:20:04 INFO]: [Skript] # /test10
[15:20:04 INFO]: IS DIAMOND SWORD
[15:20:04 INFO]: [Skript] # test10 took 0.439022 milliseconds

Another thing:
If I give this sword to player give diamond sword of sharpness 5, knockback 2 named "TEST" with lore "HEY" to player, then use player has diamond sword of sharpness 5, knockback 2 named "TEST" it won't work, because the lore doesn't match.

The same works for NBT, if I add ANYTHING different in NBT, it won't match the item, even though it matches the conditions.

Using only name and enchantments, it'll match normally.

inventory of %player% is empty doesn't work too.

That's it basically.

@bensku
Copy link
Member

bensku commented Aug 5, 2017

I will take look at performance issues. About your "another thing", are you sure it is new with latest release? Sounds like old "feature" for me.

@TrademarkTM
Copy link
Author

http://prntscr.com/g4sg78

command /test11:
	trigger:
		give a diamond sword of sharpness 5, knockback 2 named "TEST" with lore "HEY" to player
		if player has diamond sword of sharpness 5, knockback 2 named "TEST":
			send "Indeed"
		else:
			send "Negative"

@Tuke-Nuke
Copy link

That's because the diamond sword of sharpness 5, knockback 2 named "TEST" isn't a part of condition, so it creates an item and pass to the condition, and the condition check if it has that item, not similiar item.

@Snow-Pyon Snow-Pyon added the priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation). label Jan 31, 2018
@TheLimeGlass
Copy link
Collaborator

@TrademarkTM Is this still an issue? Bensku has changed the Slot system recently

@bensku
Copy link
Member

bensku commented May 31, 2018

The changes are not relevant for this issue.

@Whimsyturtle Whimsyturtle added the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Jul 10, 2020
@sovdeeth
Copy link
Member

sovdeeth commented Jul 31, 2023

Tested on 2.7-beta3, 1.20.1:

[22:28:54 INFO]: [Skript] # /test0
[22:28:54 INFO]: [Skript] # test0 took 1.3959 milliseconds
[22:28:37 INFO]: [Skript] # /test1
[22:28:37 INFO]: [Skript] # test1 took 0.1617 milliseconds
[22:28:40 INFO]: [Skript] # /test3
[22:28:40 INFO]: [Skript] # test3 took 0.5178 milliseconds
[22:28:42 INFO]: [Skript] # /test4
[22:28:42 INFO]: [Skript] # test4 took 0.1335 milliseconds
[22:29:52 INFO]: [Skript] # /test5
[22:29:52 INFO]: [Skript] # test5 took 0.6364 milliseconds
[22:30:35 INFO]: [Skript] # /test6
[22:30:35 INFO]: CAN HOLD STONE
[22:30:35 INFO]: [Skript] # test6 took 0.2426 milliseconds
[22:32:31 INFO]: [Skript] # /test8
[22:32:31 INFO]: 5
[22:32:31 INFO]: [Skript] # test8 took 0.6906 milliseconds
[22:33:18 INFO]: [Skript] # /test8
[22:33:18 INFO]: 36
[22:33:18 INFO]: [Skript] # test8 took 1.5131 milliseconds
[22:34:08 INFO]: [Skript] # /test9
[22:34:08 INFO]: [Skript] # test9 took 0.3518 milliseconds
[22:34:27 INFO]: [Skript] # /test10
[22:34:27 INFO]: [Skript] # test10 took 0.0839 milliseconds
[22:34:29 INFO]: [Skript] # /test10
[22:34:29 INFO]: IS DIAMOND SWORD
[22:34:29 INFO]: [Skript] # test10 took 0.3762 milliseconds

It seems to me the main issue is item comparison performance, not inventory manipulation.

@AyhamAl-Ali
Copy link
Member

Since we dropped support for legacy versions (1.12-) and there has been many improvements to item comparison I guess this has been improved/fixed on the way. If anyone thinks this is still a problem please open a new issue.

@AyhamAl-Ali AyhamAl-Ali added the completed The issue has been fully resolved and the change will be in the next Skript update. label Jul 31, 2023
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. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation).
Projects
None yet
Development

No branches or pull requests

8 participants