osrf / vrx

Virtual RobotX (VRX) resources.
Apache License 2.0
421 stars 189 forks source link

default value initialization to avoid redundant variable assignment #678

Closed tejalbarnwal closed 1 year ago

tejalbarnwal commented 1 year ago

Sub-Tasks

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.

M1chaelM commented 1 year ago

For now, please review this using the testing instructions above to make sure the sydney_regatta.sdf is working properly.

M1chaelM commented 1 year ago

@tejalbarnwal This seems to be working fine for syndey_regatta.sdf

tejalbarnwal commented 1 year ago

@tejalbarnwal This seems to be working fine for syndey_regatta.sdf

great, I will go ahead and push the updated world files

tejalbarnwal commented 1 year ago

update: I have updated all the world files

M1chaelM commented 1 year ago

@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?

tejalbarnwal commented 1 year ago

@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?

M1chaelM commented 1 year ago

@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!

tejalbarnwal commented 1 year ago

@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.

M1chaelM commented 1 year ago

@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.

tejalbarnwal commented 1 year ago

@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!