default value initialization to avoid redundant variable assignment #678
Conversation
|
For now, please review this using the testing instructions above to make sure the sydney_regatta.sdf is working properly. |
|
@tejalbarnwal This seems to be working fine for |
great, I will go ahead and push the updated world files |
|
update: I have updated all the world files |
vrx_gz/src/Wavefield.cc
Outdated
| @@ -28,6 +28,8 @@ | |||
| #include <gz/math/Vector2.hh> | |||
| #include <gz/math/Vector3.hh> | |||
|
|
|||
| #include <iostream> | |||
There was a problem hiding this comment.
sorry, I forgot to remove that
I used it to debug initially, will remove it
vrx_gz/src/Wavefield.cc
Outdated
| this->data->model = params["model"].string_value(); | ||
| this->data->gain = params["gain"].double_value(); | ||
| this->data->tau = params["tau"].double_value(); | ||
| if (params.count("size") > 0){ |
There was a problem hiding this comment.
nitpick: Could you move the open bracket to the next line to be consistent with the Gazebo style?
if (params.count("size") > 0)
{
this->data->size = {params["size"].vector3d_value().x(),
params["size"].vector3d_value().y()};
}
There was a problem hiding this comment.
sure, i will update them
| value { | ||
| type: DOUBLE | ||
| double_value: 2 | ||
| double_value: 0.5 |
There was a problem hiding this comment.
Is this change on purpose? It's usually 0.
There was a problem hiding this comment.
Apologies for the oversight, i forgot to reset the value back to 0 after conducting the tuning experiments, resulting in its unchanged state. I will update it in with a commit.
|
@tejalbarnwal Is this PR also intended to address issue #684 (which we discussed in our meeting but I only just wrote up today)? If so, we should wait to merge it until we figure out what the new parameters should be. Otherwise we can merge this now and create a new PR for #684. What would you prefer? |
We could merge this and create a new PR to address #684; this way, we could have a clean commit log for the repo. I was also thinking if I should squash all the commits made to this branch to just one commit? |
|
@tejalbarnwal Ok, in that case I'll make a new PR soon with the new wavefield values. Could you review it today? I don't usually squash commits but I don't think it's a problem if you prefer that. Edit: Also, since this is approved and we aren't modifying it further and we're wrapping up for the release today, it would help if you could merge it soon. Thanks! |
Yes, I will test out the parameters specified wave model envelope in #684. I will let you know in a few minutes if any warnings or error is raised on my system Oh, I wasn't aware that I could merge it. I am used to reviewers merging it in other repos. I will merge it now. |
|
@tejalbarnwal Thanks and no problem. We let the creator of the PR perform the merge (if you have the permissions, which you do). You are welcome to play around with the wave parameters, but I will also soon have a PR for you to review with specific values for the various practice worlds. |
yup, I will review it! |
Sub-Tasks
wavefield.ccto initialize the wavefield parameter with default values and update them only when a user passes a value for it in the SDF file.Summary
The modification avoids repeated initialization of some wavefield variables, which always have the same value across all world files. Therefore now, the publisher allows the user to indicate solely the variable they wish to modify, leaving the remaining variables to be managed automatically.
Test it
To test this, one can use the
sydney_regatta.sdf. The world file currently only publishes direction, steepness, gain, and period.