Skip to content
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

Make sure the player is in the vehicle before running rosPubMsg #29

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

tckarenchiang
Copy link
Contributor

This is to fix #25

-- else
-- print(instance_veh.cameraNode)
-- if the player is not in the vehicle, print error and return
if not vehicle then
Copy link
Member

@gavanderhoorn gavanderhoorn Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will avoid the nil related error, but should self.rosPubMsg also not be reset to false in this case?

Otherwise I believe ModROS:update(..) will try to still publish data, right?

Here:

FS19_modROS/modROS.lua

Lines 104 to 119 in cf452a6

function ModROS:update(dt)
-- self.dt = self.dt + dt
-- create TFMessage object
self.tf_msg = tf2_msgs_TFMessage:init()
if self.doPubMsg then
-- avoid writing to the pipe if it isn't actually open
if self.file_pipe then
self:publish_sim_time_func()
self:publish_veh_func()
self:publish_laser_scan_func()
self:publish_imu_func()
self:publish_tf()
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in that case we should probably also guard the opening of the pipe file, otherwise that will be attempted multiple times.

Copy link
Contributor Author

@tckarenchiang tckarenchiang Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But 0f5e401 also avoids publishing data if the player is not in a vehicle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why even continue with rosPubMsg(..) if it's clear the player is not in a vehicle?

By adding more conditionals everywhere it looks like we're just complicating things which could be kept simple.

Copy link
Contributor Author

@tckarenchiang tckarenchiang Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because initially I wanted to control the inactive vehicles as well, as mentioned in #3. but yes, let's fix the current issue for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are you planning to change anything, or should we merge this version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we should just merge this version for now. Even though it's not a proper fix, it does solve the issue I believe.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 4, 2021

For future development it might be best to decouple "being in a vehicle" from initialising the publishing infrastructure.

That would seem to be necessary for #3 anyway.

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's address the immediate issue.

@gavanderhoorn
Copy link
Member

@tckarenchiang: this will need a rebase I believe.

@tckarenchiang
Copy link
Contributor Author

@tckarenchiang: this will need a rebase I believe.

@tckarenchiang
Copy link
Contributor Author

sorry I accidentally clicked close with comment

@tckarenchiang tckarenchiang merged commit 3b2540e into master Jun 4, 2021
@gavanderhoorn gavanderhoorn deleted the fix_nil_vehicle branch June 4, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

rosPubMsg: attempt to index field 'vehicle' (a nil value)
2 participants