search

tomassedovic / tcod-rs

Rust bindings for libtcod 1.6.3 (the Doryen library/roguelike toolkit)
Do What The F*ck You Want To Public License
229 stars 45 forks source link

Sync/Send for Map #265

Closed fadeddata closed 6 years ago

fadeddata commented 6 years ago

I needed this change so that I could add Map as a Write<'a Map> resource using specs (https://github.com/slide-rs/specs). It's been working great and I have not witnessed any sort of issues. Is there any reason this couldn't be merged?

Thanks!

fadeddata commented 6 years ago

@L3nn0x I hope I did that right. Let me know and I'll make any required changes.

zsparal commented 6 years ago

I wouldn't add Arc<Mutex<_>> in the library itself. You can make it Send and if people need to access it from multiple threads then they can just use a Mutex<Map>

L3nn0x commented 6 years ago

I second @Gustorn. Sorry I wasn't clear enough, but I don't think the library should support that. Libtcod was never thread safe so I don't think we should support it either. Maybe we should make the library more rustacean friendly, it's something to think about.

fadeddata commented 6 years ago

What about starting a safe module that wraps all of the API's we might want to have wrapped in Arc<Mutex<_>> ?

L3nn0x commented 6 years ago

I don't know where @tomassedovic wants to go with this library, but it could be a possibility. Feel free to open an issue about it so we can discuss it more in depth.

zsparal commented 6 years ago

I feel like a big part of the Rust philosophy is zero-cost abstractions, adding Arc<Mutex<_>> around everything is way too restrictive:

  1. For one, users of the library might not want to use the types in a multithreaded context
  2. They might use multiple threads but opt for a different thread synchronization method

In both of these cases, users of the library would be paying for the thread-safe abstractions for no reason. I think wrapping your own types if you need to is a perfectly valid way to go about things: consider that the Rust standard library isn't thread-safe by default either (with some notable exceptions of course).

L3nn0x commented 6 years ago

@fadeddata just realized I already had an issue about it open here: https://github.com/tomassedovic/tcod-rs/issues/259 I was looking into using Box to store the raw pointers, but I haven't had the time to do a poc. That would make the library more "rustacean". (I'm not even sure it could work). But I think we should do more research before committing to something :)