-
Notifications
You must be signed in to change notification settings - Fork 17
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
Delete BlackBoard
struct.
#38
Conversation
An additional idea: what if we just delete this field entirely? We already require that the user passes in a mutable closure. Users could just as easily store their blackboard data as a reference in the closure. This would simplify our API a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, but thanks for chasing down improvements/optimizations :) A general guideline is that we'd like to keep the library as simple and lean as possible with the least amount of deps. Also if introducing any new behaviors is generally not favored unless it is strictly needed and the that the existing behaviors combined does not suffice
This struct doesn't do anything special and is just a newtype without providing anything. I believe in the past this did more, but now it is just a needless wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Before approving, please address any comments and bump minor version again since we are changing the API (I don't want us to bump major version to v1)
@@ -60,7 +60,7 @@ pub fn game_tick(dt: f32, cursor: mint::Point2<f32>, boid: &mut Boid, other_boid | |||
|
|||
// unwrap bt for boid | |||
let mut bt = boid.bt.clone(); | |||
let db = &*bt.get_blackboard().get_db(); | |||
let db = &*bt.get_blackboard(); | |||
let win_width: f32 = *db.get("win_width").unwrap(); | |||
let win_height: f32 = *db.get("win_height").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you check here if the &* which is first a reference () followed by a dereference (&) on line 63, followed by a reference again () on line 64 and 65 is redundant? I think this can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also found another case in the 3d example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, bumping the minor version is correct.
In my own crates, I normally bump the minor version after I've done all my refactors rather than inline with them. Slightly different workflow, my bad!
@@ -60,7 +60,7 @@ pub fn game_tick(dt: f32, cursor: mint::Point2<f32>, boid: &mut Boid, other_boid | |||
|
|||
// unwrap bt for boid | |||
let mut bt = boid.bt.clone(); | |||
let db = &*bt.get_blackboard().get_db(); | |||
let db = &*bt.get_blackboard(); | |||
let win_width: f32 = *db.get("win_width").unwrap(); | |||
let win_height: f32 = *db.get("win_height").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also found another case in the 3d example.
This struct doesn't do anything special and is just a newtype without providing anything. I believe in the past this did more, but now it is just a needless wrapper.