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

feat(cmd_blueprint): add blueprint_select action #3881

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

salinecitrine
Copy link
Collaborator

This action directly selects a blueprint, either by index or name. It can be bound like this examples:

bind           Alt+sc_p  blueprint_select 3 // select index 3
bind           Alt+sc_p  blueprint_select wind // select name "wind"

While name works, it also can only be set by modifying blueprints.json by hand.

Addresses Issue(s)

Test steps

  • bind the action; the examples above should be sufficient
  • Valid actions
    • with no unit selected, press the hotkey; the blueprint should be selected but not activated
    • with a builder selected, and a valid index or name passed, press the hotkey; the blueprint should be selected and the blueprint placement command activated
  • Invalid actions
    • with an invalid index (larger than the number of blueprints) passed, press the hotkey; nothing should happen
    • with an invalid name (not present in any blueprint; this will almost always be the case) passed, press the hotkey; nothing should happen

This action directly selects a blueprint, either by index or name. It
can be bound like this examples:

```
bind           Alt+sc_p  blueprint_select 3 // select index 3
bind           Alt+sc_p  blueprint_select wind // select name "wind"
```

While name works, it also can only be set by modifying blueprints.json
by hand.
@saurtron
Copy link
Contributor

saurtron commented Oct 26, 2024

  • Code looks ok.
  • Ingame test ok for me (bound both by name and index and did work).

Just one nitpick: I feel like the case where it's not working should echo some feedback, otherwise it can be confusing for users when they misstype the name or index and it's not working.

(it happened to me since initially I set the name to "wind" -with actual quotations, when it needs to be without quotations, I just read the example too fast).

On the other hand all the echoing by this widget when something visible is happening (basically the "selected blueprint..." one) should go eventually but that's out of scope for this PR (in that case the echo would still be ok when seleting under the hood like with this action).

thx for making this :).

Comment on lines +270 to 272
if index ~= nil and index > 0 then
Spring.Echo("[Blueprint] selected blueprint #" .. selectedBlueprintIndex)
end
Copy link
Member

Choose a reason for hiding this comment

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

Is this echo needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case where blueprint placement is active, it's a bit redundant. If it's not (selecting while inactive is a new case this PR introduces), then it's the only indication that something happened. I could imagine removing a lot of the Spring.Echos; I do feel like actions should give some feedback to the player though. I'm not sure what the best way to do that is.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make it not echo at least when the blueprint gui is open. It's a good compromise imo, since some feedback when nothing visible is seen is good.

Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind echos are for dev work, not for anything player-facing.

Comment on lines +895 to +900
for blueprintIndex, blueprint in ipairs(blueprints) do
if blueprint.name == targetName then
selectIndex(blueprintIndex)
return true
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to compile a map of blueprint name -> blueprint ID, so they can be selected directly by name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would probably be a bit faster, it would just need a bit more code, and performance shouldn't be an issue here. I'm ambivalent which way it's done, so I don't mind changing it.

@WatchTheFort
Copy link
Member

Could also do a case-insensitive match on blueprint name, to avoid casing errors entirely

@salinecitrine
Copy link
Collaborator Author

  • Code looks ok.
  • Ingame test ok for me (bound both by name and index and did work).

Just one nitpick: I feel like the case where it's not working should echo some feedback, otherwise it can be confusing for users when they misstype the name or index and it's not working.

(it happened to me since initially I set the name to "wind" -with actual quotations, when it needs to be without quotations, I just read the example too fast).

On the other hand all the echoing by this widget when something visible is happening (basically the "selected blueprint..." one) should go eventually but that's out of scope for this PR (in that case the echo would still be ok when seleting under the hood like with this action).

thx for making this :).

Thanks for the review! I mostly agree on the echos; another UX feedback mechanism would probably be better, but I'm not sure how else to do it. I can add an echo for the failure case, since that one does seem pretty important (with the caveat that just like other feedback it should probably eventually not be an echo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants