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

[wpilibNewCommands] Add get subsystem list function from CommandScheduler #7835

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

OhmV-IR
Copy link
Contributor

@OhmV-IR OhmV-IR commented Feb 28, 2025

In java, the subsystems are returned as an immutable set and in c++ they are returned as a const vector. No tests for these as there isn't testing framework to create mock subsystems in wpilibNewCommands tests, at least not that I'm aware of.

@OhmV-IR OhmV-IR requested a review from a team as a code owner February 28, 2025 05:07
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@github-actions github-actions bot added the component: command-based WPILib Command Based Library label Feb 28, 2025
@OhmV-IR OhmV-IR changed the title Add get subsystem list function from CommandScheduler [wpilibNewCommands] Add get subsystem list function from CommandScheduler Feb 28, 2025
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Is the intent to do a copy or return an object that is a readonly view? (Does the below work?)

Set<Subsystem> subsystems = CommandScheduler.getInstance().getAllRegisteredSubsystems();
Subsystem mySubsystem = new Subsystem() {};
CommandScheduler.getInstance.registerSubsystem(mySubsystem);
assertTrue(subsystems.contains(mySubsystem));

As it is right now, Java is a readonly view (the code snippet above will pass) while C++ is a copy (the C++ version of the above will not pass).

See #6475 for discussion of this feature and #6526 for the troubles encountered a year ago in getting the readonly view to work in C++.

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Feb 28, 2025

robotpycommands2 pr: robotpy/robotpy-commands-v2#86

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Feb 28, 2025

Ideally it will be a readonly view in c++, as that makes the most sense but might have to make both copies if it turns out that we can't get the readonly view to compile in cpp

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Mar 12, 2025

sorry for inactivity, have some time now, just letting you know so this doesn't get closed

@TheTripleV
Copy link
Member

Is UnmodifiableSet ordered like .keySet?

The return value should be ordered

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Mar 17, 2025

Unfortunately it's looking like std::ranges::keys_view is incompatible with DenseMap, so there's either the option of writing some sort of backend to support it(which will likely be pretty inefficient), or returning a copy and changing java to do the same. I'm getting this output trying to build: https://gist.github.com/OhmV-IR/42e21bf0b6de95cc59cbcaabb4ff4e93
The output makes me feel like I'm missing an include, but I'm pretty sure keys view is under the #include which I have in both my CommandScheduler.cpp and CommandScheduler.h
See below code snippets:
std::views::keys<wpi::DenseMap<Subsystem*, std::unique_ptr<Command>>> CommandScheduler::GetAllRegisteredSubsystems() { return std::views::keys(m_impl->subsystems); }
std::views::keys<wpi::DenseMap<Subsystem*, std::unique_ptr<Command>>> GetAllRegisteredSubsystems();

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Mar 17, 2025

I changed java to also return a copy instead of a read-only view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: command-based WPILib Command Based Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants