scan API restructure

[petermoz]

Problem

There are a lot of parameters to the scan functions scan_advanced and scan_range. This makes code harder to read and maintain and because each parameter has a default value led to some bugs where parameters were accidentally missing.

An example function call is:

blensor.blendodyne.scan_advanced( angle_resolution=obj.velodyne_angle_resolution, 
            max_distance=obj.velodyne_max_dist, start_angle=obj.velodyne_start_angle, 
            end_angle=obj.velodyne_end_angle, noise_mu = obj.velodyne_noise_mu, 
            noise_sigma=obj.velodyne_noise_sigma, add_blender_mesh=obj.add_scan_mesh, 
            add_noisy_blender_mesh=obj.add_noise_scan_mesh, 
            rotation_speed = obj.velodyne_rotation_speed, evd_file=filename,
            world_transformation = world_transformation )

Possible Solution

As can be seen above, most of the parameters are from the sensor object obj. Why not just pass this as a parameter instead? The scan_advanced interface would then become something like

def scan_advanced(sensor, evd_file, world_transform):

Potential Problems

I'm guessing that the reason for the explicit parameters was that so that scan_advanced could be run without having a sensor object. A workaround for this in the proposed interface would be wrapping parameters in a dummy class.

class Namespace(object):
    pass  

sensor = Namespace()
sensor.velodyne_angle_resolution = 0.1
sensor.max_distance = 120
...
scan_advanced(sensor, 'test.evd', Matrix())

Other than testing, for which the dummy class is probably ok, I can't think of any use cases that require the parameters to be separated as they are.

Conclusion

If think that the parameters should be left as they are, feel free to close this issue. If, however, you're happy with the proposal, I'll go ahead and do it, and submit the patch as a pull request.

[mgschwan]

I do agree that the parameters with their default parameters are messy, this is "historical" since the whole thing emerged from a single python script to simulate the velodyne.

Getting rid of this is definitely a worthy task, but you are right. one reason for not passing the obj itself is the fact that this code can be called from other python scripts that just use the active camera as the scanner.

Using some kind of inheritance for sensor parameters however seems to be very useful

Proposal

Create a Sensor class that combines the default parameters and has a constructor that takes a Camera object and copies the relevant data into the sensor object.
Derive all sensors and extend the parameters (change the default values) accordingly.

class Sensor(object):
  max_distance = 120
  def __init__(self, blender_object):
    self.max_distance = blender_object.max_distance

class Velodyne(Sensor):
  rotation_speed = 15
  angle_resolution = 0.1728
  ...
  ...
  def __init__(self, blender_object):
    Sensor.__init__(self, blender_object)
    self.rotation_speed = blender_object.rotation_speed
    self.angle_resolution = blender_object.angle_resolution
...
...
...

sensor = Velodyne(obj)

scan_advanced(sensor,'test.evd', Matrix())

[petermoz]

I think copying from obj to sensor is redundant because if you just want to use the active camera it will already have the default values set, thanks to the addProperties(cType) call. Restricting default values to the one place (that addProperties method) would be cleaner, and removes any potential issues with forgetting to copy from obj to sensor if we add a new property.

Using your proposal, if you wished to create a "sensor" without modifying the active camera, you could do:

 sensor = Velodyne()
 sensor.max_dist = 100
 scan_advanced(sensor, ...)

But with mine the only difference would be

 sensor = bpy.data.objects.new('temp', None)
 sensor.max_dist = 100
 scan_advanced(sensor, ...)

Perhaps I'll code it up and if you find any use cases that need the separate Sensor classes, it'd be trivial to add in. The added advantage of my method is that the other properties already defined on sensor (such as object_matrix, etc.) are also accessible

[mgschwan]

Hmm there could theoretically be a scenario where the sensor objects store information that is not stored as blender properties.

For example:
The laser configurations should be stored in the sensor object since we could have more than one sensor of the same type. Some properties may be loaded from a file instead of exposing them in the blender menu.

If a sensor is it's own object it could have save/load methods to store the sensor configuration in a file.

Thinking further into the future i think the addProperties should be done by the Sensor object too. That way the default values would only be held in one place.

But we can go with your approach for now and check if the requirements i mentioned are really an issue or not

[petermoz]

Well currently laser configurations are stored in each object, and that would remain the case. So different sensors could be represented by different cameras in the scene. If you wanted to be able to load parameters from a file into a blender object, it'd be easy to write an operator to do it.

As far as further into the future goes, I think it makes sense for the Sensor object to be a subclass of bpy.types.object (or just remain an object with extra properties like it is now). That way blender will save it for you automatically, and also allow cool things like keyframe animation of parameters.

I'll get started on the refactoring and see what issues I run into on the way :)

[mgschwan]

I know, storing the sensor properties in the blender file greatly simplifies the distribution of scans without having to send the whole scan result.

Well then it's decided. :)