Title: Hands-On Rust (Chapter 10)
This has been bugging me a bit since coding it, so I tried the change and it seems to work.
Isn’t it a bit expensive to clone a HashSet that you intend to immediately replace?
I tried the following change to clone_dirty(&self) -> Self
pub fn clone_dirty(&self) -> Self {
// let mut cloned = self.clone(); // TODO: isn't this expensive??
// cloned.is_dirty = true;
// cloned
Self {
visible_tiles: HashSet::new(),
radius: self.radius,
is_dirty: true,
}
}
Game play seems to be working fine so far. I’m assuming that creating a new HashSet is less expensive than closing a populated HashSet. That also raised the question for me as to whether visible_tiles should be Option instead and the None would serve the same purpose as is_dirty. Maybe I’ll see that this is a bad idea as I progress…
I tried the Option (and remove (is_dirty)) change. It originally made a few checks more verbose/painful. Then, because of the repetition, I added
pub fn is_visible(&self, pt: &Point) -> bool {
match &self.visible_tiles {
Some(vt) => vt.contains(pt),
None => false,
}
}
to the FieldOfView struct impl, and used that. All seems to be working fine. I suppose that the risk is that I’ll get a false on is visible when I would, previously have gotten a true when querying the “dirty” HashSet. I suspect, though, that using the results of querying a dirty HashSet is more likely to be a bug.
That’s a good point; clone can be expensive (it’s remarkable how well LLVM optimizes it out, especially on release builds). There’s a bit of compiler magic that notices that it’s basically a set of memcpy calls under the hood, and elides them. I like your approach better, though - it’s dangerous to rely on the compiler noticing an optimization when you can tell it what you want.
Adding an is_visible method is also a great idea. I’ll see if I can refactor the book code a little.
I’ll try and merge this into the next beta. Thank you!
