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

Enhancement: Consider Node Ancestry in Proto Type Restrictions #6574

Conversation

CoolSpy3
Copy link
Contributor

@CoolSpy3 CoolSpy3 commented Jul 3, 2024

Description
Currently if a SFNode or MFNode field is value-restricted, any nodes added to the field, must exactly match the restricted type (or in the case of proto-nodes, directly derive from it). This PR updates the behavior so that the entire ancestry of the nodes are searched. (ex. A field restricted to Pose nodes can now also contain Solid nodes) See the linked issue for more information.

A couple of notes about decisions made in this PR:

  • Abstract node classes cannot be represented in the current VRML implementation. Thus, where they occur in the ancestry tree, they have been omitted or jumped-over. Allowing them would require a larger rewrite of the parsing code, which I do not feel is necessary at this time.
    • As a side effect of this choice, there are no cases of multiple-inheritance. I originally planed to implement this to deal with cases such as SolidDevice, but rolled it back in favor of #CS41. This means that some of the code will have to be reworked if multiple-inheritance is desired in the future, however, the changes are fairly straight-forward, so I don't have any concerns.
  • In order to implement this PR, I had to add a method to determine which nodes defined by Webots extended other nodes. I chose to do this with an additional comment in the wrl files. In my mind this makes the most sense because I would expect this information to be contained with the WbNodeModel object, which is generated from these files. The only other ways I could think of doing this would be either writing out all of the relationships in a file (which would be more verbose and detached from the actual node definitions) or keeping track of constructor invocations at runtime (which would need to be done on every node instance creation rather than on every model instance creation and would add additional c++ code to convey information which is already expressed in the inheritance structure of the classes. The c++ heirarchy also includes the abstract nodes mentioned above. It would not make sense for these to be included, which is unclear from the c++ code, but is clearer when viewed in the wrl context).
  • Many of the protos in this project could now be simplified with these changes in mind. I did not do that here because 1) the new behavior is backward compatible in 99% of cases (i.e. definitions which are now technically overexplicit will still function without errors), and 2) it may be helpful, especially in places such as the reference docs, for new developers to see multiple examples of nodes that can be used in a specific field even if a more generic definition would suffice.
  • There is currently an edge case which causes Transform nodes to be converted to a Pose during loading. This could disrupt this system in the case where someone restricts a field to a Transform. There are a couple solutions (short of removing the linked code), which all have drawbacks. If I was to fix this, I would do it by interpreting a Transform restriction as a Pose restriction. However, this would add a bit of extra clutter to the code, and given that 1) I doubt many people are restricting nodes to Transform, and 2) this behavior would've already been present in previous versions, I've opted to just ignore it.

If you feel that any of these decisions should be reconsidered, let me know, and I can update the appropriate code.

Related Issues
This pull-request fixes issue #6573.

Tasks

  • Add a method for querying the node hierarchy at runtime
  • Update type checking code to search the ancestry tree
  • Update the test suite
  • Update the documentation
  • Update the changelog

Documentation
PROTO Definition#field-value-restriction

@CoolSpy3 CoolSpy3 marked this pull request as ready for review July 3, 2024 09:43
@CoolSpy3 CoolSpy3 requested a review from a team as a code owner July 3, 2024 09:43
@CoolSpy3 CoolSpy3 added the test suite Start the test suite label Jul 3, 2024
@CoolSpy3
Copy link
Contributor Author

CoolSpy3 commented Jul 3, 2024

Note: The test suite is going to fail due to a bug in the display test. This is unrelated to this PR and also occurs on develop. The important thing is that that parser tests pass.

@omichel omichel self-requested a review July 3, 2024 11:45
@CoolSpy3 CoolSpy3 marked this pull request as draft July 6, 2024 07:51
@CoolSpy3
Copy link
Contributor Author

CoolSpy3 commented Jul 6, 2024

Alright, I've updated the code to implement the new parsing behavior. I also updated a bunch of other instances where field values are validated (that I missed before) to point to the new implementation. I still have to test the = syntax manually (and then write automatic tests) and update the docs. I've updated the task list to reflect this, and I'll mark this PR as a draft until that gets completed.

@omichel
Copy link
Member

omichel commented Jul 6, 2024

That looks good to me. I do have a preference for the + syntax which is backwards compatible as the old syntax would still work as before and the + syntax would add more possibilities, e.g., allowing also derived nodes.

@CoolSpy3
Copy link
Contributor Author

CoolSpy3 commented Jul 8, 2024

Alright, I think this is ready for review again! It looks like the type name and the following {} are parsed collectively as a node, so in order to not have a weird edge case in the node parsing code, I switched to the syntax to

field SFNode{Solid{}+,Transform{}+} shape Solid{}

Notes:

  • When testing locally, you have to delete resources/proto-list.xml before your first build in order to regenerate it. (I modified the generation script to include some extra info.)
  • While debugging I discovered a bug in some of the modified functions which prevents a node from being added to an unconnectedField from the GUI. This is not a regression, so I do not believe it has to be addressed in this PR. If you agree, I can migrate it to an issue,

Edit: Hmmm... Looks like some of the tests are failing. I'll do some investigating.

Edit 2: I think I might've fixed it. Testing...

@CoolSpy3 CoolSpy3 marked this pull request as ready for review July 8, 2024 21:02
@CoolSpy3
Copy link
Contributor Author

CoolSpy3 commented Jul 9, 2024

Tests are fixed!

@omichel omichel self-requested a review July 10, 2024 06:24
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.

Very good job! Thank you.
I would only request a couple of minor changes.

src/webots/vrml/WbProtoModel.cpp Outdated Show resolved Hide resolved
tests/manual_tests/protos/DerivedPoseProto.proto Outdated Show resolved Hide resolved
Co-authored-by: Olivier Michel <[email protected]>
@omichel omichel self-requested a review July 10, 2024 21:02
@omichel omichel merged commit 7ddffc6 into cyberbotics:develop Jul 11, 2024
17 of 20 checks passed
@CoolSpy3 CoolSpy3 deleted the enhancement-consider-node-ancestry-in-proto-type-restrictions branch July 29, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

Node Ancestry Should be Considered for Node Type Restrictions in PROTOs
2 participants