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

implemented Clone trait for Surface #136

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Xirdus
Copy link
Contributor

@Xirdus Xirdus commented Aug 3, 2014

In this implementation, clone() does deep copy of surface, resulting in independent duplicated pixel data. Alternatively, it could be done using refcounting (SDL already supports that), but it would be counterintuitive when blitting on the copied surface, or using copy-on-write, but this would be much harder to implement.

@lifthrasiir
Copy link
Collaborator

Sorry for late reply. I think we should go the same interface as std::rc::Rc, where .clone() means a shallow copy with only refcount increasing and .make_unique() (somehow) means a deep copy. (An implicit copy-on-write is not much used in Rust since it hides the internal cost.) What do you think about this?

@Xirdus
Copy link
Contributor Author

Xirdus commented Aug 7, 2014

Well, the problem is, when you blit something on refcounted surface (using SDL's integrated refcounting mechanics), all other shallow copies get overwritten too. We could make some sort of copy-on-write, but this would require additional control structure (we could use SDL_Surface's existing refcount and do copy on blit if rc>1, but this would make us drift away from how SDL handles this value).

I think deep copy makes more sense; std::rc::Rc is a handle with refcount, so "Rc.clone()" technically means "make a deep copy of the handle" - which results in underlying object's shallow copy. I don't think of rust-sdl's surface as a handle, but as a surface itself - so deep copy seemed more natural.

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