`mjs_nextElement` does not search recursively correctly

[JewelryForge]

Intro

Hi!

I am a MuJoCo user working on legged locomotion.

My setup

latest MuJoCo 8a825e6050e88b113db03b2cd102e5cb711d4c61 C API OS: Ubuntu 24.04

What's happening? What did you expect?

After revision ae4987026c9d9a8bfc590c0865cc6bb6db2980be, mjs_firstElement and mjs_nextElement can be used to iterate all elements of the whole mjSpec with the specified type recursively.

Take the following code as an example. In my opinion, it should output

Body : world
Body : base
Body : FL_hip
Body : FR_hip
Body : RL_hip
Body : RR_hip
Body : FL_thigh
Body : FR_thigh
Body : RL_thigh
Body : RR_thigh
Body : FL_calf
Body : FR_calf
Body : RL_calf
Body : RR_calf

However, the latest MuJoCo outputs an incomplete result:

Body : base
Body : FL_hip
Body : FR_hip
Body : RL_hip
Body : RR_hip
Body : FL_thigh
Body : FL_calf

Additionally, I wonder if it would be better if the elements are searched depth-first, in the same order as the compiled mjModel. It may output the below result in this example.

Body : world
Body : base
Body : FL_hip
Body : FL_thigh
Body : FL_calf
Body : FR_hip
Body : FR_thigh
Body : FR_calf
Body : RL_hip
Body : RL_thigh
Body : RL_calf
Body : RR_hip
Body : RR_thigh
Body : RR_calf

Steps for reproduction

Change the directory of mujoco_menagerie to the actual directory below and run the below code.

Minimal model for reproduction

mujoco_menagerie/unitree_go2/go2.xml can be used for reproduction.

Code required for reproduction

  std::array<char, 1000> error_buf{};
  auto *spec = mj_parseXML("mujoco_menagerie/unitree_go2/go2.xml", nullptr, error_buf.data(), error_buf.size());
  if (spec == nullptr) {
    std::cerr << "mj_parseXML Error: " << error_buf.data() << std::endl;
    return 1;
  }

  for (auto *element = mjs_firstElement(spec, mjtObj::mjOBJ_BODY);
       element != nullptr; element = mjs_nextElement(spec, element)) {
    auto *jnt = mjs_asBody(element);
    std::cout << "Body : " << mjs_getString(jnt->name) << std::endl;
  }

Confirmations

• [X] I searched the latest documentation thoroughly before posting.
• [X] I searched previous Issues and Discussions, I am certain this has not been raised before.

[JewelryForge]

By the way, I'm confused by this code segment

https://github.com/google-deepmind/mujoco/blob/8a825e6050e88b113db03b2cd102e5cb711d4c61/src/user/user_objects.cc#L1294-L1296

Does it just mean

    if (!bodies.empty()) {
      return bodies[0]->NextChild(NULL, child->elemtype, true);
    }

or a mistake?

[quagla]

Hi, I'm about to push out the option to do breadth-first iteration. Incidentally I was already working on it while you opened the bug :)

By the way, I'm confused by this code segment

https://github.com/google-deepmind/mujoco/blob/8a825e6050e88b113db03b2cd102e5cb711d4c61/src/user/user_objects.cc#L1294-L1296

Does it just mean

    if (!bodies.empty()) {
      return bodies[0]->NextChild(NULL, child->elemtype, true);
    }

or a mistake?

The next child can be in bodies[0] or in another body of bodies. The other body is not a child of body[0] so how would you reach it?

[JewelryForge]

Thank you for your hard work! I'm sorry I do not understand what you mean by

The next child can be in bodies[0] or in another body of bodies. The other body is not a child of body[0] so how would you reach it?

I want to express that the two code segments may be equivalent because the function returns in the first iteration of the for-loop if the bodies is not empty. So the for-loop does not have effect and can be simplified to a if sentense.

[quagla]

Oh I see what you mean. I will take a look, thank you.

[JewelryForge]

I'm sorry but I don't think the patch has fixed this issue. The key is that the recursive search produces an incomplete result and the wrong result can be also reproduced after the patch.

Neither 89aea5 nor dfe8e4 fixes this issue.

[quagla]

I'm sorry but I don't think the patch has fixed this issue. The key is that the recursive search produces an incomplete result and the wrong result can be also reproduced after the patch.

Neither 89aea5 nor dfe8e4 fixes this issue.

The second commit was a typo. I meant to type 2111, sorry for that.