Skip to content

[Issue-126] fix several issues with animate - #131

Merged
jcao-bdai merged 4 commits into
masterfrom
issue-126/fix-animate-behavior
Jul 29, 2024
Merged

jcao-bdai merged 4 commits into
masterfrom
issue-126/fix-animate-behavior

Conversation

@jcao-bdai

@jcao-bdai jcao-bdai commented Jul 19, 2024 •

Copy link
Copy Markdown
Contributor

Fix several issues related to #126

  • expose "ax" and "dims" arguments to animate(...), to be consistent with other API's;
  • update animate implementation to be consistent with trajectory frame's data type, in particular, SE3Array / SO3Array types instead of SE3 / SO3, in line with animate.run(...)'s implementation.

Example use-cases:

...
ax = fig.add_subplot(111, projection='3d')
T1.plot(ax=ax, dims=[-5, 10])
T1.animate(start=T2, ax=ax)  # would use the existing ax which is [-5, 10]
# T1.animate(start=T2, ax=ax, dims=[-10, 10])  # would use the existing ax and override its lims to [-10, 10]
plt.show()

@jcao-bdai
jcao-bdai force-pushed the issue-126/fix-animate-behavior branch 2 times, most recently from 6dc40ae to 21f9b56 Compare July 19, 2024 20:36
@jcao-bdai
jcao-bdai force-pushed the issue-126/fix-animate-behavior branch from 2e32897 to 3262923 Compare July 22, 2024 16:47
@jcao-bdai
jcao-bdai force-pushed the issue-126/fix-animate-behavior branch from 3262923 to cc7398f Compare July 23, 2024 19:13

@bhung-bdai bhung-bdai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like a reasonable change to me, but I'm not a primary user of the Spatialmath animation tool. I would feel more comfortable if there was infra to unit test, but I'm also not Spatialmath comes with that in the first place.

@myeatman-bdai myeatman-bdai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved, minor comment.

# # ax.set_aspect('equal')
ax = smb.plotvol3(ax=ax, dim=dim)
if dim is not None:
if len(dim) == 2:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we have a runtime error that check's for length 2 or 6?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks! added this check in 27fd4aa

@jcao-bdai
jcao-bdai merged commit d6134bf into master Jul 29, 2024
@jcao-bdai
jcao-bdai deleted the issue-126/fix-animate-behavior branch July 29, 2024 18:31
petercorke added a commit that referenced this pull request Sep 15, 2026
## Summary

`tranimate(T, dim=1.5)` — using the scalar-shorthand `dim` convention
established by `plotvol2`/`plotvol3` elsewhere in this codebase —
currently
crashes with:

```
TypeError: spatialmath.base.animate.Animate() got multiple values for keyword argument 'dim'
```

Two compounding bugs, both fixed here:

1. `tranimate()` popped `"dims"` (plural) out of `kwargs` before
forwarding
   to `Animate(dim=..., **kwargs)`, but every other function in this
codebase (`plotvol2`, `plotvol3`, `Animate.__init__` itself) names the
   parameter `dim` (singular). A caller correctly using `dim=` per that
   convention never got popped, so it stayed in `**kwargs` and collided
   with the explicit `dim=dim` already being passed — hence "multiple
   values for keyword argument 'dim'".
2. Even with the right keyword, `Animate.__init__` had its own
hand-rolled
   length check that only accepted a 2- or 6-element `dim`, silently
   dropping the scalar-shorthand convention (`A` → `[-A, A]` on every
   axis) that `plotvol3` already supports via the shared `expand_dims()`
   helper — `Animate` just never called it.

## Fix

- `tranimate()` now accepts `dim`, with `dims` kept as a back-compat
alias
  for existing callers following the old docstring examples.
- `Animate.__init__` now calls `expand_dims(dim, nd=3)` (the same helper
`plotvol3` uses) instead of its own incomplete validation, so scalar and
  2-element shorthand work consistently everywhere.

## Regression context

This isn't a long-standing issue — it's a regression from #131 (`Jien
Cao`,
2024-07-29, fixing #126), which *introduced* both the `dims` rename and
the
new strict length check while trying to "expose `ax` and `dims`
arguments
to animate(...), to be consistent with other APIs." Before that commit,
`tranimate` passed `**kwargs` straight through to `Animate`, and scalar
`dim` worked correctly via `Animate`'s existing `plotvol3` delegation.

## Test plan

- [x] Verified directly: `Animate(dim=1.5)`, `Animate(dim=[0,5])`,
      `tranimate(R, dim=1.5, movie=True)`, and `tranimate(R, dims=[0,5],
      movie=True)` (back-compat alias) all construct/run correctly
- [x] Full existing test suite: 342 passed, no regressions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants