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

ActorMap is missing read-only API #205

Closed
rlidwka opened this issue Jul 14, 2023 · 2 comments
Closed

ActorMap is missing read-only API #205

rlidwka opened this issue Jul 14, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@rlidwka
Copy link
Contributor

rlidwka commented Jul 14, 2023

Description of the problem

I'm trying to register on_wake_sleep callback, which has the following signature:

pub trait WakeSleepCallback<L: ArticulationLink, S: RigidStatic, D: RigidDynamic>: Sized {
    fn on_wake_sleep(&mut self, actors: &[&ActorMap<L, S, D>], is_waking: bool);
}

ActorMap as of now has the following implementation (link):

impl<L, S, D> ActorMap<L, S, D>
where
    L: ArticulationLink,
    S: RigidStatic,
    D: RigidDynamic,
{
    pub fn cast_map<'a, Ret, ALFn, RSFn, RDFn>(
        &'a mut self,
        mut articulation_link_fn: ALFn,
        mut rigid_static_fn: RSFn,
        mut rigid_dynamic_fn: RDFn,
    ) -> Ret {/*...*/}

    pub fn as_rigid_dynamic(&mut self) -> Option<&mut D> {/*...*/}
    pub fn as_rigid_static(&mut self) -> Option<&mut S> {/*...*/}
    pub fn as_articulation_link(&mut self) -> Option<&mut L> {/*...*/}
}

Please notice that every single method in ActorMap requires &mut self, but WakeSleepCallback only gives access to &self, thus making wake_sleep callback unusable as is.

Any physx user can of course cast pointers by herself similar to how ActorMap does it internally, but that's not ideal.

Proposed solution

  1. rename existing cast_map and as_* to cast_map_mut and as_*_mut respectively.
  2. add cast_map and as_* functions which take &self and return Option<&self>

For example:

// instead of this
pub fn as_rigid_dynamic(&mut self) -> Option<&mut D> {/*...*/}

// suggesting to do this
pub fn as_rigid_dynamic(&self) -> Option<&D> {/*...*/}
pub fn as_rigid_dynamic_mut(&mut self) -> Option<&mut D> {/*...*/}
@rlidwka rlidwka added the enhancement New feature or request label Jul 14, 2023
@tgolsson
Copy link
Member

Seems sensible for me as described here -- feel free to open a PR with the changes.

I noticed this callback is set up in the examples but never actually used. If you have a use-case, can you maybe extend the examples (ball_physx.rs)? Would help catch regressions in the future too.

rlidwka added a commit to rlidwka/physx-rs that referenced this issue Jul 17, 2023
@rlidwka
Copy link
Contributor Author

rlidwka commented Jul 17, 2023

Seems sensible for me as described here -- feel free to open a PR with the changes.

done

I noticed this callback is set up in the examples but never actually used. If you have a use-case, can you maybe extend the examples (ball_physx.rs)?

There is no actual use-case yet.

I can put a debug-print whenever ball is put to sleep, but I'm not sure how useful it would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants