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

Improve Node Field Query API #6613

Conversation

CoolSpy3
Copy link
Contributor

@CoolSpy3 CoolSpy3 commented Aug 3, 2024

Description
Allows accessing the internal fields of a proto hierarchy from a supervisor controller. There's still a lot of work to be done to integrate this into Webots, but now that I have a working proof-of-concept, I'm opening this PR to keep track of the remaining tasks.

This PR introduces the following new supervisor API functions:

  • wb_supervisor_node_get_proto
  • wb_supervisor_proto_get_type_name
  • wb_supervisor_proto_get_parent
  • wb_supervisor_proto_get_field
  • wb_supervisor_proto_get_field_by_index
  • wb_supervisor_proto_get_number_of_fields
  • wb_supervisor_proto_is_derived

These act on the new type WbProtoRef.

Additionally, the following function has been added for WbFieldRef objects:

  • wb_supervisor_field_get_actual_field

Finally, the following functions have been renamed for clarity:

  • wb_supervisor_node_get_proto_field -> wb_supervisor_node_get_base_node_field
  • wb_supervisor_node_get_proto_field_by_index -> wb_supervisor_node_get_base_node_field_by_index
  • wb_supervisor_node_get_number_of_proto_fields -> wb_supervisor_node_get_number_of_base_node_fields

UPDATE (Aug 19): After looking through the usages of the above methods, I've noticed that the wb_supervisor_node_get_field (and related) methods are used much more than the proto variants. Thus, I've decided to keep its functionality the same in order to minimize the amount of code that will need to be updated in existing controllers. I've renamed the other methods appropriately (and updated the above text). To see the originals, check the edit history of this comment.

Related Issues
This pull-request fixes issue #6354.

Tasks

  • Allow Webots to keep track of internal proto parameters
  • Allow internal proto parameters to be accessed from the C supervisor API
  • Ensure that updates to internal proto fields are propagated to the supervisor
  • Add tests for the new API methods
  • Propagate supervisor API changes to other languages
    • Update the C++ API
    • Update the Java API
    • Update the Python API
    • Update the Matlab API
    • Update WbLanguage.cpp with the new api methods
  • Update references to the renamed API methods throughout the codebase
  • [ ] Add a sample usage demo After looking at the existing samples, I found that there is currently no sample for and of the wb_supervisor_node_get_proto_field methods. Thus, I'm not sure what I good sample for a purely introspective API would look like. For the moment, I'll assume that the advice to write samples is indented for more simulation-facing features (e.g. devices).
  • Update ROS
  • Update the documentation
    • Update documentation relating to the renamed methods and clarify the distinction between them
    • Add documentation for the new API methods
  • Update the changelog

Documentation
Supervisor API

@CoolSpy3 CoolSpy3 added the feature Implementation of a major feature label Aug 3, 2024
(this shouldn't be necessary because if the parent ref is invalidated, the child should've been as well, but it's good practice)
@brettle
Copy link
Contributor

brettle commented Sep 7, 2024

Also, for whomever ends up reviewing this, do you think WbNode.cpp:1795 could delete field references being held by WbNodeProtoInfo? It seems like it might, but I haven't yet found a test case that causes that behavior to occur (that includes running the current tests [which should cover all scenarios] with hidden fields [instead of normal/unconnected fields]). I would think using a hidden field in SolidProtoHierarchyInternal would cause this, but it doesn't. (Conversely, the tests fail without the current change to WbNode:1782). If you don't want to investigate, I'm happy enough with saying "the tests should cover all use cases" and seeing if a bug pops up later.

I'm ok with relying on the test coverage.

brettle
brettle previously approved these changes Sep 7, 2024
Copy link
Contributor

@brettle brettle left a comment

Choose a reason for hiding this comment

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

Excellent work!

@brettle
Copy link
Contributor

brettle commented Sep 7, 2024

@omichel, I've reviewed and approved but am not a maintainer so you, or someone else who is, needs to approve in order to get it merged.

Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Thank you!
And thanks also to @brettle for the nice review!

@CoolSpy3 CoolSpy3 merged commit 7f9f56a into cyberbotics:develop Sep 7, 2024
22 checks passed
@CoolSpy3 CoolSpy3 deleted the feature-improve-node-field-query-api branch September 7, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implementation of a major feature test ros Start the ros test test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

Improve Node Field Query API
3 participants