why is this changed

I calculate that dynamically in the constructor, since it depends on self.dt and before it was forcing it to always have this value anyway:

Previous code:

assert (
int(np.round(1.0 / self.dt)) == self.metadata["render_fps"]
), f'Expected value: {int(np.round(1.0 / self.dt))}, Actual value: {self.metadata["render_fps"]}'

Here is the functionality for verifying that render_fps equals the expected value is removed (for v4 or older based environments that define the render_fps themselfs)

Hmm right, but if render_fps is in metadata and is different than int(np.round(1.0 / self.dt)), the assertion fails. We can maintain the previous behavior by always overriding render_fps with the computed value for the moment then, perhaps?

No, it should fail if render_fps is explicitly defined to something else, otherwise render issues will arise

Hmm ok!
Then I can modify it to set this computed value if render_fps is not in the metadata, and run the assertion if it is?

yes

Should I also update the individual Mujoco environments to remove the metadata -where it matches the new automatically generated one- ?

Just leave all the v4 and older environment as is

ok!

if we need, wouldn't be easier to do

    metadata = {
        "render_modes": [
            "human",
            "rgb_array",
            "depth_array",
        ],
    "render_fps": int(np.round(1.0 / self.dt))
    }
    ```

Hmm I am not sure 'self.dt' can be called from there, as `metadata' would be a static class variable?

That is also why problems arise if metadata' is not copied before replacing values (in the new code) -> if derived classes don't specifically override metadata', the same (possibly wrong, in case of `render_fps') values end up shared between them.

sorry meant to say, there is no need to have a metadata static class variable

    self.metadata = {
        "render_modes": [
            "human",
            "rgb_array",
            "depth_array",
        ],
    "render_fps": int(np.round(1.0 / self.dt))
    }

Yes, that was my original intention. ;)
However, if metadata is initialized as self.metadata in the constructor (as in this approach), but a derived class still implements the older way with metadata as static class variable, the outcome may be unexpected for the user (hence the use of assertions in the latest code, in case for some reason a mismatch happens).

I can test that and add a pytest to check for correct behavior in case a derived class re-defines the metadata as static (sub)class variable, as is currently done.
In any case, the current main repository does not allow for any derived class to have any different metadata, so any metadata written explicitly by a user (for a new derived class) must be identical to this value).

It would also help to remove all 'metadata' from the existing Mujoco environments to improve clarity, and indeed only define it once in the base class. Or perhaps to change them from class variables to object variables initialized in the constructor (i.e. "render_fps" can be always removed, since there is only one valid value; "render_modes" can be set to replace the default array, e.g.

self.metadata["render_modes"] = [new-modes]

eploiting the fact that the default metadata would always contain the two keys, "render_modes" and "render_fps").

** This applying to MujocoEnv and derived classes.
gym.Env also (ab)uses static class variables, including metadata (although it is not strict like MujocoEnv in the choice of "render_fps") ?

In that case, if metadata is kept a class variable as in gym.Env, then dynamic copying (metadata.copy()) and updating of "render_fps" the constructor may be the only alternative.

By the way, "render_fps" is only used either explicitly by a few environments that handle their own rendering, or as argument to be explicitly passed by the user to the `save_video' utility function.

It does not seem to be a problem to just set the correct value (for Mujoco envs) without checks on later modifications by the users. Perhaps we can add a comment in the code to explain that Mujoco environments should leave the default "render_fps" as is, since there is no use in ever modifying it.