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

Add "last commands" and "command count" getters #1725

Open
sprunk opened this issue Oct 9, 2024 · 5 comments
Open

Add "last commands" and "command count" getters #1725

sprunk opened this issue Oct 9, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers Lua API

Comments

@sprunk
Copy link
Collaborator

sprunk commented Oct 9, 2024

  • if you pass 0 to Spring.GetUnitCommands, it will return a number with the length of the queue instead of a table with commands like it does with any other argument. Add dedicated Spring.GetUnitCommandCount(unitID) -> number for this (and perhaps mark the former for deprecation).
  • add Spring.GetUnitCurrentCommand(unitID, -n) negative index support to return nth from last. Right now to get the last command you have to get the number of commands first and subtract manually.
@saurtron
Copy link
Collaborator

Btw, why is there also the alias GetCommandQueue for GetUnitCommands?

I also think many times just the cmdID is needed, so GetUnitCurrentCommandID (or maybe just GetUnitCommandID) should be added too, taking a number or negative number, to fetch that command id from the queue.

Also, I feel GetUnitCurrentCommand but accepting an index is a bit of a misnomer since one would expect it's really about the current command. Maybe should be simply GetUnitCommand, not totally sure tbh, but maybe someone has a better idea.

@sprunk
Copy link
Collaborator Author

sprunk commented Dec 13, 2024

why is there also the alias GetCommandQueue for GetUnitCommands?

Legacy. Perhaps make PRs to games to replace GetCommandQueue and then we will be able to remove it.

feel GetUnitCurrentCommand but accepting an index is a bit of a misnomer since one would expect it's really about the current command (...) Maybe should be simply GetUnitCommand

Also legacy. Originally it had no n parameter and could only get the 1st command, the index param was added later. Adding an alias sounds fine.

@saurtron
Copy link
Collaborator

why is there also the alias GetCommandQueue for GetUnitCommands?

Legacy. Perhaps make PRs to games to replace GetCommandQueue and then we will be able to remove it.

feel GetUnitCurrentCommand but accepting an index is a bit of a misnomer since one would expect it's really about the current command (...) Maybe should be simply GetUnitCommand

Also legacy. Originally it had no n parameter and could only get the 1st command, the index param was added later. Adding an alias sounds fine.

One of them should be deprecated, imo there should be just one name for functions otherwise it becomes very confusing.

After deprecating then it's easy to make a PR to change all obsolete uses since it's just changing the name.

@sprunk
Copy link
Collaborator Author

sprunk commented Dec 13, 2024

It's already just changing the name, no need to wait.

@saurtron
Copy link
Collaborator

saurtron commented Dec 13, 2024

It's already just changing the name, no need to wait.

I'll deprecate GetCommandQueue if you think that's the bad one, otherwise what's the justification for the games to pick up the change?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Lua API
Projects
None yet
Development

No branches or pull requests

2 participants