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

custom attachments #20

Merged
merged 13 commits into from
Nov 12, 2023
Merged

custom attachments #20

merged 13 commits into from
Nov 12, 2023

Conversation

brandon-reinhart
Copy link
Collaborator

In progress PR adding the ability to create custom attachments that can be assigned to slots.

May optionally include the ability to create custom attachment loaders.

@brandon-reinhart
Copy link
Collaborator Author

I am working on the interface to modify a region attachment after it has been created. The region attachment can be empty upon creation, but to render any visible contents, the user needs to set all fields and call update_region. This should be clear and not something you can mess up.

@brandon-reinhart
Copy link
Collaborator Author

spSlot_setAttachment: It appears that setting the attachment on the slot does not increment the attachment ref count.

This seems strange to me, as the pointer is in use and if an attachment is set on a skin the ref count is incremented. It seems to me that an attachment that is in use by a slot should have its refcount increment and dispose called when it is no longer in use, but spine c does not do this.

As a result, it is possible to set an attachment on a slot and then drop the attachment, causing an access violation.

@brandon-reinhart
Copy link
Collaborator Author

brandon-reinhart commented Oct 23, 2023

Another issue I am investigating is that calling Slot::set_attachment cause an immediate panic. Calling the underlying c::spSlot_setAttachment works fine.

Edit: This is due to the non-incrementing ref count. If the refcount is incremented by spSlot_setAttachment, Slot::set_attachment is reliable.

@brandon-reinhart
Copy link
Collaborator Author

@brandon-reinhart
Copy link
Collaborator Author

An alternative to making attachment accessors mutable is to place the create attachment convenience function in RegionAttachment and have it take the loader and props as arguments.

@brandon-reinhart
Copy link
Collaborator Author

brandon-reinhart commented Oct 23, 2023

Example of the current code:

        let attachment_loader = AttachmentLoader::new_atlas_loader(&atlas);

        let region_props = RegionProps {
            x: 0.0,
            y: 0.0,
            scale_x: 1.0,
            scale_y: 1.0,
            rotation: 0.0,
            width: 128.0,
            height: 128.0,
            color: Color::new_rgba(1.0, 1.0, 1.0, 1.0),
        };

        let attachment = attachment_loader
            .create_region_attachment(None, "test", "head", &region_props)
            .unwrap();

        unsafe {
            let mut slot = controller.skeleton.find_slot_mut("front-shin").unwrap();
            slot.set_attachment(Some(attachment.clone()));
        }

@jabuwu
Copy link
Owner

jabuwu commented Oct 23, 2023

Nice work.

spSlot_setAttachment: It appears that setting the attachment on the slot does not increment the attachment ref count.

We shouldn't be making modifications directly to spine_c.rs, since it's auto-generated. We will need to wait for a response to the Spine C PRs.


$(#[$($attrss)*])*
pub fn $rust_set(&mut self, value: String) {
let c_str = std::ffi::CString::new(value).expect("Null byte found in provided string");
Copy link
Owner

Choose a reason for hiding this comment

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

I realize it's an exceedingly rare case, but I think we should bubble up the error here, wrapped in SpineError::NulError.

/// # Errors
///
/// Returns [`AttachmentLoaderError::CreateAttachmentFailed`] if creating the attachment failed.
/// Check error1() and error2() for more information.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// Check error1() and error2() for more information.
/// Check [`error1`](`Self::error1`) and [`error2`](`Self::error2`) for more information.

src/attachment_loader.rs Outdated Show resolved Hide resolved
@brandon-reinhart
Copy link
Collaborator Author

Nice work.

spSlot_setAttachment: It appears that setting the attachment on the slot does not increment the attachment ref count.

We shouldn't be making modifications directly to spine_c.rs, since it's auto-generated. We will need to wait for a response to the Spine C PRs.

Yeah, that's why I noted this is temporary. The goal of this was to prove out it resolves the problems, which it does in my testing.

As far as I can tell, this is safe to do if the attachment is valid, once esoteric updates spine with the ref count increment in spSlot_setAttachment
@brandon-reinhart
Copy link
Collaborator Author

This is all usable. Anything else I should do here? It doesn't require any changes from the spine runtime.

@brandon-reinhart
Copy link
Collaborator Author

Ship it!

@jabuwu jabuwu merged commit 2199add into jabuwu:main Nov 12, 2023
8 checks passed
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.

2 participants