danieljfarrell / pvtrace

Optical ray tracing for luminescent materials and spectral converter photovoltaic devices
Other
97 stars 94 forks source link

Light mask functions slightly inconsistent with geometry sizes #47

Open peter-spencer opened 3 years ago

peter-spencer commented 3 years ago

There is a slight inconsistency between the Light mask functions, e.g. cube_mask vs geometry.Box

Calling Box(X,Y,Z) creates a box with sides of length X, Y, and Z respectively.

Calling pvtrace.light.light.cube_mask(X,Y,Z) returns a random coordinate between (-X --> +X,-Y --> +Y, -Z --> +Z) which is a volume equivalent to Box((2X,2Y,2*Z)).

This works well because the Box() is centred on (0,0,0) by default, so the resulting Box() and Light() behave the same when transformations are applied to move and rotate them.

I had at first assumed that the coordinates supplied to Light() behaved exactly like Box() but this was my mistake as the code is clearly commented; I thought I'd flag this potential confusion for other users. I'm not sure whether it needs fixing because changing the behaviour could break existing code.

Perhaps an optional future feature could be added to make a geometry object into a light, or use as a template for a light source?

danieljfarrell commented 3 years ago

I think this makes sense for cube_mask. This would avoid having lots of 2* in your python scripts.

The rect_mask(X, Y) definition should change too to match it.

Or maybe to preserve backwards compatibility we introduce a new mask type with optional arguments. This can no longer be a delegate function type, it would need to be an object which implements __call__. For example,

# Example 1
class BoxMask(object):
    def __init__(self, x=0, y=0, z=0):
        super(BoxMask, self).__init__()
        self.x = x
        self.y = y
        self.z = z

    def __call__(self):
        # return ray here!

# Now this looks a lot more like the Box geometry
box_mask = BoxMask(x=1, y=1, z=1)

# Now you can call the object just like a function to generate rays
ray = box_mask()

Nice feature is that if initialised with:

A way of using the box geometry directly would be to do something like (in pseudo-code):

# Example 2
class Mask(object):
    def __init__(self, geometry):
        self.geometry = geometry

    def __call__(self):
        if self.geometry == "is a box":
            # call cube_mask(....)

box_mask = Mask(Box(1,1,1))
ray = box_mask()

But now you cannot generate rays over lines and planes because pvtrace only supports 3D objects! So this is very elegant if and only if you want to generate rays over a volume.

So although it's annoying to introduce another API which looks like it does the same thing. I think in this case it's better to keep the them separate objects but unify how they are initialised i.e. BoxMask example 1.

Finally, I'm not too sure that mask is the correct name but I never settled on anything better! A mask covers an area so you cannot see it, this kind of does the opposite: it defines an area we want to use. Also mask is a very 2D word, but here we are generating 0D--3D.

danieljfarrell commented 3 years ago

Thinking again about improving this API, how about the word constraint rather than mask?

For example, when making the light source node, rather than,

Light(position=functools.partial(rectangular_mask, 1, 1))

it would be,

Light(position=constraint(xmin=-1, xmax=1, ymin=-1, ymax=1))

Here I'm making it look like a function, but really it would return an object,


class CartesianConstraint(object):
    def __init__(self, xmin=None, xmax=None, ymin=None, ymax=None, zmin=None, zmax=None):
        super(CartesianConstraint, self).__init__()
        self.xmin = xmin
        self.ymin = ymin
        self.zmin = zmin
        self.xmax = xmax
        self.ymax = ymax
        self.zmax = zmax

    def __call__(self):
        # return ray here

def contraint(xmin=None, xmax=None, ymin=None, ymax=None, zmin=None, zmax=None):
    return CartesianConstraint(...)
danieljfarrell commented 3 years ago

Thinking about this again...

I think I now like simply, "inside" i.e.

Light(position=inside(xmin=-1, xmax=1, ymin=-1, ymax=1))