SkriptLang / Skript

Skript is a Spigot plugin which allows server admins to customize their server easily, but without the hassle of programming a plugin or asking/paying someone to program a plugin for them.
https://docs.skriptlang.org
GNU General Public License v3.0
1.07k stars 370 forks source link

Indexes being automatically sorted? #371

Open Snow-Pyon opened 7 years ago

Snow-Pyon commented 7 years ago

Server version: 1.11.2 (Spigot) Skript version: Skript 2.2 dev23

Plugins: LuckPerms, Skript, MundoSK, skDragon, skUtilities, Test, Reqn, SkQuery, Vault, RandomSK, Fe, ProtocolLib, SkStuff, Skellett, skRayFall, Umbaska, TuSKe, SharpSK, SharpSKUpdater.

Issue: Hey, I'm trying to do something like a item parser which parses the items from a YML file and I just realized with this code:

loop yml nodes "%{_id}%.attributes" of file "plugins/Test/test.yml":
    loop yml nodes "%{_id}%.attributes.%loop-value-2%" of file "plugins/Test/test.yml":
        set {_item::attributes::%loop-value-2%::%loop-value-3%} to yml value "%{_id}%.attributes.%loop-value-2%.%loop-value-3%" of file "plugins/Test/test.yml"

Here that part of the YML file:

  attributes:
    1:
      AttributeName: 'generic.attackDamage'
      Name: 'generic.attackDamage'
      Amount: 50
      Operation: 0
      UUIDLeast: 10
      UUIDMost: 5

that's all ok but when I loop the following list with this code:

loop tree of {_item::attributes::*}:
    broadcast "%branch% == %loop-value%"
    set {_index::*} to split "%branch%" by "::"
    if {_nbt} isn't set:
        set {_nbt} to "%{_index::1}%:{%{_index::2}%:%loop-value%"
    else:
        set {_nbt} to "%{_nbt}%,%{_index::2}%:%loop-value%"

The broadcast is:

1::amount == 50
1::attributename == generic.attackDamage
1::name == generic.attackDamage
1::operation == 0
1::uuidleast == 10
1::uuidmost == 5

when it should be:

1::attributename == generic.attackDamage
1::name == generic.attackDamage
1::amount == 50
1::operation == 0
1::uuidmost == 5
1::uuidleast == 10

And it's not an issue of the tree of %objects% expression because I tried just looping the list {_item::attributes::1::*} and it's the same output.

P.S: other issue is that the indexes are converted to lowercase but that's known problem.

TheBentoBox commented 7 years ago

I've had this issue as well; I have an auction script on my server where players are added to a queue list, and sometimes players will get stuck in queue hell and never have their auction run because getting the first element of %list% doesn't return the actual first element (the oldest thing in the list) but rather some seemingly random player, which is probably actually the first one alphabetically. It's nice to see someone else confirming similar behavior.

This may have to do with the sorted %objects% expression which was added? I don't see why it'd happen on its own, but it certainly seems to be. Bug regardless. Thank you for the detailed report as always.

Tuke-Nuke commented 7 years ago

Isn't it by default in Skript?Cause Skript sorts all variables numerically/alphabetically. And It happens with indexes too.

TheBentoBox commented 7 years ago

Well if that's the case, now that there's a sorted %objects% expression, maybe it shouldn't try to sort all variables automatically. That's not how lists work in any other programming language, and it'd mean there's no good way to use lists as a FIFO system.

I guess this is more dev-needed though, since I'm not sure if this is something Bensku would consider a bug or an intended feature.

Tuke-Nuke commented 7 years ago

Yeah, it might be interesting to see, but it would conflict with some stuffs: The variables are stored in a Object where the order of indexes (variable's name) is randomly, which means in some moment Skript sorts the variables (or if it does...). And I never saw a easy way to make an expression in Skript to sort a variable with its indexes. Maybe it will depend if someone else has an idea, as Bensku has more important things to do.

JeepSmash commented 7 years ago

If anyone ever opened their variables.scv file they would easily see that even lists are not stored as objects but just as individual variables in a database file. When ever a variable is looked up it can make an extreme difference in processing time required if the variables are stored alphabetically as opposed to non alphabetic. The most efficient way I can think of to store and process a database is when it's sorted by the most important value. So i would assume every variable is automatically placed within the database in a quasi-alphabetic order the instant it's created. This would make life really easy and sorting irrelevant since its an automated part of its operation. If variable names are created with forethought into this auto sorting then creating objects with multiple values that make up that object then any single aspect of the object can be easily looped in order by their index.

When I say quasi-alphabetic i mean that its not completely alphabetic. When list indexes are stored they are not stored alphabeticaly but numerically assuming there is no letters in the index and strictly numbers. Alphabetically would index 10 between 1 and 2 but Skript stores 10 between 9 and 11 in numeric order.

Also worth noting that when adding values to a list like: add "jeepsmash" to {users::*} will search for integer indexes to place the value in starting with 1. If 1 doesnt exist then it will create {users::1}="jeepsmash" even if indexes 2 3 4 and 5 already exist. So as BentoBox mentioned if you add values to a list without a specified index the lowest positive integer index is automatically assigned to the value. This would not be a problem unless values were being removed from the list as they are used which then opens up a free spot in the list which the next added value would then occupy. So in his example of an auction plugin queue it would fill any opened index with the next added entry which would put looping priority to the newest added item in the queue over older values assuming they are being removed as quickly as they are added. To get around this undesired effect always specify your index and i usually have a variable that holds the next index for most of my lists like:

if {nextuserindex} isn't set:
  set {nextuserindex} to 1
set {user::%{nextuserindex}%} to "jeepsmash"
add 1 to {nextuserindex}

This way everytime you loop your list it will always return the values in the order they were added to the list rather than in a seemingly random order you would get if values were constantly being added/removed from the list at varying rates.

As BentoBox also mentioned this isnt how any other programming language stores variables and thats very true but essential in the way Skript is designed to "read" like a language. Pointers and references would not make for easy reading. Other languages also dont store their variables/objects in databases while in use but direct in memory which requires addressing by their location within the memory as opposed to addressing by an arbitrary name assigned to every saved piece of data as Skript does.

Sorry if my description might be confusing to read but the basics of what i'm trying to explain is that alphabetizing is a necessary part of the way variables are stored to increase processing variable lookups and that I think this is the best possible way it could be done. Adjusting your code to compensate for this in certain circumstances is relatively simple and straightforward.

TheBentoBox commented 7 years ago

Awesome description. I didn't find it confusing and understood it fully, and learned a lot in the process. I'll use your advice to fix my queue issue.

In that case, I'm not sure this is really a bug that should be addressed then, for the reasons you've stated. But I'll give time for more people to read and respond to your reply.

bensku commented 7 years ago

@JeepSmash is right. I don't fully understand it either, but when looping, data is in sorted form. However, sorted expression will take this form, then re-sort it by more strict criteria, and return a list (i.e. number keys) of values.

I'm open to change, if it comes as pull request and is extensively tested.

bloggy commented 7 years ago

Please bensku, can you fix this?

command /order:
    trigger:
        add "player1" to {_test::*}
        add "player2" to {_test::*}
        add "player3" to {_test::*}

        broadcast "1.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

        wait 1 second
        broadcast "deleting player1"          
        loop {_test::*}:
            if loop-value is "player1":
                remove loop-value from {_test::*}

        broadcast "2.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

        wait 1 second
        broadcast "adding player1"
        add "player1" to {_test::*}

        broadcast "3.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

Result: 1.) List: player1 player2 player3

2.) List: player2 player3

3.) List: player1 player2 player3

But List 3.) should be:

player2 player3 player1

because I added player1 after I have added player2 + player3.

Why is this and how can I fix it???

I think automatically ordering of list variable elements is a no go. It would break everything.

JeepSmash commented 7 years ago

Your Fixed Code:

command /order:
    trigger:
        set {index} to 1
        set {_test::%{index}%} to "player1"
        add 1 to {index}
        set {_test::%{index}%} to "player2"
        add 1 to {index}
        set {_test::%{index}%} to "player3"
        add 1 to {index}

        broadcast "1.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

        wait 1 second
        broadcast "deleting player1"          
        loop {_test::*}:
            if loop-value is "player1":
                delete {_test::%{loop-index}%}

        broadcast "2.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

        wait 1 second
        broadcast "adding player1"
        set {_test::%{index}%} to "player1"
        add 1 to {index}

        broadcast "3.) List:"
        loop {_test::*}:
            broadcast "%loop-value%"

All you need to do is keep an index for you list and increment it everytime you add a value to your list at the index. Then you can remove and add new values and they will always stay in the order they were chronologically added to the list.

bloggy commented 7 years ago

Thanks for the idea but that can't be the solution. List variables should not be sorted automatically! For me this seems to be a bug.

Syst3ms commented 7 years ago

See @JeepSmash 's explanation : "player2" and "player3" have indexes 2 and 3. When you add something to a list, it tries to see if an index '1' exists. If so, the '1' index is associated with the value. So, since index '1' is vacant when you remove "player1", if you add "player1" again, it will automatically be coupled with the '1' index

bloggy commented 7 years ago

I really understand this, but however, there should be a simple built-in solution to add a value to a list variable without getting it sorted that way.

Snow-Pyon commented 7 years ago

The problem is that there's not @bloggy, you'll have to use the Mash's workaround because I don't think this will be changed soon.

bloggy commented 7 years ago

I am using the workaround for now. But what is the "sorted {list::*}" expression for then?

Syst3ms commented 7 years ago

@Kazuma-san Actually, my PR fixes a part of it (indexes being sorted), but it could cause issue to the database and to some scripts, so no global change.

TheBentoBox commented 7 years ago

@bloggy Skript automatically sorts by index, in the order of:

{list::1}
{list::2}
...
{list::9}

And if you added two more things, so it added objects at indices 10 and 11, it'd sort it "alphabetically", making it like this:

{list::1}
{list::10}
{list::11}
{list::2}
...
{list::9}

The sorted %objects% expression returns the objects sorted by value. So if you had a list that was as follows:

{list::someIndex} == 13
{list::1} == 5
{list::DORP} == 9

Sorting it would return it like this:

{list::1} == 5
{list::2} == 9
{list::3} == 13

The values are now ordered, and it also doesn't preserve the keys.

bloggy commented 7 years ago

For Toplists we'd need a sorted function that keeps the indexes. Hope it can be added soon, I think many people would use it. ALso sorting needs the option to have it sorted "highest to lowest" and "lowest to highest"

Snow-Pyon commented 7 years ago

@bloggy not necessary, it's really easy to invert a list.

bloggy commented 7 years ago

But how? Why not add a simple function like:

sort {list::*} by index from highest to lowest
sort {list::*} by value from to lowest to highest
Syst3ms commented 7 years ago

Because lists are always sorted by indexes, the indexes cannot be sorted themselves.