-
Notifications
You must be signed in to change notification settings - Fork 22
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
try to backport the named parameter patch #79
Conversation
I'm still not getting the whole arrows example for physics rendered even with this patch.. |
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.
I didn't know parameters could be unnamed, it's good to use the provided UUIDs as IDs instead when we can. However, I requesteth changes
.get(param_name) | ||
.map(|pn| self.parameters.get(pn).expect("param index mismatch {pn}")) | ||
.unwrap_or_else(|| panic!("No parameter named: {}", param_name)); |
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.
.expect
is not a format-like macro, it's just a function. So that {pn}
is gonna display as {pn}
and not the contents of pn
. I suggest either removing that part, or (better solution) make get/set_param
return a Result<T, ParamNotFound>
, which will automatically also make the methods more resilient like we wanted a few days ago
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.
Ehh yeah I wasn't paying attention to this so it might already be a Result in the other branch ? I'll check and copy
The inner error is a panic error though, the index having a reference to a non-existent value
Just realized parameters already have use the UUID in the |
TLDR; I'm not really convinced this should be merged but I'm also not convinced the physics and main branch haven't diverged so much that a rebase will be just as much work as a rewrite |
Physics is now fixed on the physics branch, so I think I'll close this |
No description provided.