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.

Sign up for GitHub

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

Add get_sprite_count, remove_sprite, get_userdata and set_userdata #60

Closed
wants to merge 4 commits into from
Closed

Add get_sprite_count, remove_sprite, get_userdata and set_userdata #60

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d86e344
add get/set userdata methods to sprite
lyonbeckers Sep 6, 2023
859c727
fixed userdata
lyonbeckers Sep 8, 2023
297ed96
added a Userdata trait, use a Rc<Box> instead so that it is easier to…
lyonbeckers Sep 9, 2023
519cb64
do not use mut reference for get
lyonbeckers Sep 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/sprite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,32 @@ extern "C" fn get_sprite_collision_response(
}

impl SpriteInner {
pub fn get_userdata<T>(&mut self) -> Result<Rc<Box<T>>, Error>
lyonbeckers marked this conversation as resolved.
Show resolved Hide resolved
where
T: Userdata,
{
let ptr =
pd_func_caller!((*self.playdate_sprite).getUserdata, self.raw_sprite)? as *const Box<T>;

let rc = unsafe { Rc::from_raw(ptr) };

unsafe { Rc::increment_strong_count(Rc::as_ptr(&rc)) }
Ok(rc)
}

pub fn set_userdata<T>(&mut self, userdata: Rc<Box<T>>) -> Result<(), Error>
where
T: Userdata,
{
let ptr = Rc::into_raw(userdata);

pd_func_caller!(
(*self.playdate_sprite).setUserdata,
self.raw_sprite,
ptr as *mut core::ffi::c_void
)
}

pub fn set_use_custom_draw(&mut self) -> Result<(), Error> {
self.set_draw_function(unsafe { SPRITE_DRAW.expect("SPRITE_DRAW") })
}
Expand Down Expand Up @@ -287,8 +313,30 @@ impl SpriteInner {
}
}

pub trait Userdata {}

impl Drop for SpriteInner {
fn drop(&mut self) {
fn free_userdata(sprite: &SpriteInner) -> Result<(), Error> {
let ptr = pd_func_caller!((*sprite.playdate_sprite).getUserdata, sprite.raw_sprite)?;

if ptr as *const _ == core::ptr::null() {
// No userdata on this sprite, nothing to do
return Ok(());
}

let rc = unsafe { Rc::from_raw(ptr as *const Box<dyn Userdata>) };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you can do this safely, since the type is not Rc<Box<dyn Userdata>>, it's Rc<Box<T>>.

The right way to solve the problem with the T generic is moving it into a parameter of SpriteInner:

pub struct SpriteInner<T> {
    pub raw_sprite: *mut crankstart_sys::LCDSprite,
    playdate_sprite: *const playdate_sprite,
    image: Option<Bitmap>,
    _userdata: PhantomData<Rc<Box<T>>>,
}

But this may not be easy to consolidate with the Sprite type that owns it.

(As a bonus, making the SpriteInner hold an Option<Rc<Box<T>> would eliminate the need to add this extra drop logic.)

I don't really know of any other way to do this. Type erasure can't work because the get/set API takes Rc. It would have to take full ownership (not just shared ownership) of the type to safely erase the type.

But maybe that is good enough? The caller can always get a userdata pointer after moving it to the sprite. It just depends on if this is acceptable for your use case, e.g. you don't require the Rc to exist before the userdata is set on the sprite. An example where this ownership model breaks down is that sprites will only be able to share userdata with double-indirection.

Copy link
Contributor Author

@lyonbeckers lyonbeckers Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think you can do this safely, since the type is not Rc<Box<dyn Userdata>>, it's Rc<Box<T>>.

Yeah I wasn't 100% certain about this one, because I figured it would be aligned to the box size, not specifically T. If I'm wrong about that, if I'm understanding correctly, I think it could become a problem if replacing the userdata with something else that has the trait but is not the same T. On that note though, I'm realizing that I should probably check the userdata ptr in set_userdata and drop the reference before setting it again if it's not null (same cleanup logic as in the drop).

Yeah this is a bit of a weird one because for my use case, the userdata exists as a component in an ECS model, the sprite just needs to reference it in the draw call. I could avoid using userdata completely by querying my world in the draw call based on the sprite's tag, but this saves a bit of work there since I can set the userdata when setting up the sprite itself.

Similarly in most cases, I'd expect that most people would find safer ways of handling something that would otherwise be userdata through the Game state object itself, bypassing Playdate's C API entirely.

I also considered SpriteInner<T>, but I figured it might be a pain to have to specify it where the majority of cases might call for no userdata at all. Sprite would have to be Sprite<T>, and you'd need to call SpriteManager::new_sprite::<T>() (Possibly just using () for when it's not needed, but still). With adding the Option<Rc<Box<T>> like you said you could just bypass the C API too and just return the owned data. Maybe that could be more universally useful to others? It'd be a breaking change for sure though

Copy link
Contributor

@parasyte parasyte Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There's a thing called object safety which requires that constructing a trait object must always be possible for every T: Trait. And because that isn't the case here (there is no trait bound on T), I think this implementation violates at least object safety.

The bigger problem, though, is that nothing implements the UserData trait. So I don't see how the compiler can correctly drop the type behind the Rc.

I actually do like the idea of type erasing the userdata safely. I think it can be done with core::any::Any to downcast out of get_userdata. But if it's behind a smart pointer and Any, I don't really know what benefit you are getting out of the FFI calls, sending a raw pointer to the SDK. You can just do the userdata in pure safe Rust at this point. And you don't need the type parameter on Sprite for it, either.

Similarly in most cases, I'd expect that most people would find safer ways of handling something that would otherwise be userdata through the Game state object itself, bypassing Playdate's C API entirely.

Yep! Basically this! 😂 I'm using an ECS, too. I would expect the Sprite to be a component as well. The problem might be that it's !Send and !Sync. Which ... basically prevents adding it to any ECS that exists, at least as a component. What I ended up doing, even though I am not using Sprite, is putting my Playdate resources into a shipyard::Unique, which can be !Send and !Sync. And they can just be put into a BTreeMap with a &'static str, String, or u32 key... Some type that can trivially be put into a component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem might be that it's !Send and !Sync

I wound up forking edict which is supposed to be no_std, but modifying it a lot 😂. I've basically just been stripping out any Send + Sync trait bounds where I can.

I'll close this because I'm sold on the possibility of doing this safely. I'm going to look into a safe implementation that uses Any, because like you said, we could avoid the type parameter which prevents any breaking changes, and I think it's just as good to sort of mimic the functionality of the C API without having to directly make the ffi call


// Just for clarity, we're dropping the rc which will decrease the strong count
drop(rc);

Ok(())
}

if let Err(err) = free_userdata(self) {
log_to_console!("error dropping userdata: {}", err);
}

pd_func_caller_log!((*self.playdate_sprite).freeSprite, self.raw_sprite);
unsafe {
if let Some(collision_responses) = SPRITE_COLLISION_RESPONSES.as_mut() {
Expand Down Expand Up @@ -318,6 +366,26 @@ pub struct Sprite {
}

impl Sprite {
pub fn get_userdata<T>(&self) -> Result<Rc<Box<T>>, Error>
where
T: Userdata,
{
self.inner
.try_borrow_mut()
.map_err(Error::msg)?
.get_userdata()
}

pub fn set_userdata<T>(&mut self, userdata: Rc<Box<T>>) -> Result<(), Error>
where
T: Userdata,
{
self.inner
.try_borrow_mut()
.map_err(Error::msg)?
.set_userdata(userdata)
}

pub fn set_use_custom_draw(&mut self) -> Result<(), Error> {
self.inner
.try_borrow_mut()
Expand Down Expand Up @@ -495,6 +563,17 @@ impl SpriteManager {
)
}

pub fn get_sprite_count(&self) -> Result<i32, Error> {
pd_func_caller!((*self.playdate_sprite).getSpriteCount)
}

pub fn remove_sprite(&self, sprite: &Sprite) -> Result<(), Error> {
pd_func_caller!(
(*self.playdate_sprite).removeSprite,
sprite.inner.borrow().raw_sprite
)
}

pub fn add_dirty_rect(dirty_rect: LCDRect) -> Result<(), Error> {
pd_func_caller!((*Self::get_mut().playdate_sprite).addDirtyRect, dirty_rect)
}
Expand Down