6
\$\begingroup\$

I am writing a simple raytracer just for fun. I want to iterate over rows, and each row must be an iterator over columns. A strategy I produced is the following:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def __iter__(self):

        def genR():
            class IterRow(object):
                def __init__(self, plane, row):
                    self.plane = plane
                    self.row = row

                def __iter__(self):
                    def genC():
                        for column in xrange(self.plane.resolution[1]):
                            origin = (self.plane.pixel_size * 
                                       (column - self.plane.resolution[0] / 2.0 + 0.5)
                                        , 
                                        self.plane.pixel_size *
                                         (self.row - self.plane.resolution[1] / 2.0 + 0.5)
                                        , 
                                        100.0
                                     )
                            yield ( Ray(origin = origin, direction = (0.0,0.0,-1.0)), 
                                    (self.row,column)
                                )
                        return
                    return genC()
                def __str__(self):
                    return "Row iterator on row "+str(self.row)
            for row in xrange(self.resolution[0]):
                yield IterRow(self, row)

        return genR()

Briefly stated: ViewPlane contains the pixel plane. This class implements __iter__ which defines and returns a generator function. This generator function yields instances of an internal class IterRow, which in turn has a __iter__ method that returns a generator iterating over the columns.

I personally think that it's difficult to understand, so I was wondering if there's a cleaner strategy to achieve the same result.

\$\endgroup\$
  • \$\begingroup\$ This is from one who isn't seasoned... why self.resolution = resolution if resolution is already available to the whole class (among others)? \$\endgroup\$ – Tshepang May 16 '11 at 5:31
  • \$\begingroup\$ Can you run pylint on your code. I see for example that you are not using self.pixel_size. Or am I missing something? \$\endgroup\$ – Tshepang May 16 '11 at 5:33
  • \$\begingroup\$ @Tshepang: I am using it \$\endgroup\$ – Stefano Borini May 16 '11 at 7:06
  • \$\begingroup\$ Do you use it is some code you are not showing? \$\endgroup\$ – Tshepang May 16 '11 at 12:16
  • \$\begingroup\$ @Tshepang: it's right there, at the instantiation of Ray. I know, it's not very readable. I am changing it \$\endgroup\$ – Stefano Borini May 16 '11 at 13:07
4
\$\begingroup\$
class Foo:
  def __iter__(self):
     def innergen():
        yield something
     return innergen()

Is the same as:

class Foo:
  def __iter__(self):
     yield something

In other words, there is no need to define the generator, just yield the values.

Defining a class inside the method just serves to make things complicated and hard to read. Define the class outside of the method.

Having return at the end of a function doesn't do anything and makes it look like you don't know what you are doing. (I'm not saying that you don't, but whenever you include code with effect it raises a red flag. Did you think it do something?)

Unless you really need to access the data row by row I suggesting flattening the iterator.

Stylistically, I prefer to break expressions up rather then trying to split them across several lines as you've done. I think it produces less clutter.

Here is my resulting code:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def __iter__(self):
        for row in xrange(self.resolution[0]):
            for column in xrange(self.resolution[1]):
                origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
                origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
                origin = (origin_x, origin_y, 100)
                ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )

                yield ray, (row, column)

I'm not familiar enough with RayTracers to know but I question having the iterator over ViewPlane produce Ray objects. An iterator is for producing the contents of the object. Is a really contained inside the ViewPlane? Or should the iterator produce coordinates and let the caller construct the Ray objects?

EDIT

Here is a version that doesn't flatten the iterator:

class ViewPlane(object):
    def __init__(self, resolution, pixel_size, gamma):
        self.resolution = resolution
        self.pixel_size = pixel_size
        self.gamma = gamma

    def row_iter(self, row):
        for column in xrange(self.resolution[1]):
            origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
            origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
            origin = (origin_x, origin_y, 100)
            ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )

            yield ray, (row, column)

    def __iter__(self):
        for row in xrange(self.resolution[0]):
            yield self.row_iter(row)
\$\endgroup\$
  • \$\begingroup\$ well, there are many things to be said. Thank you for your precious feedback. I'll answer you point by point. \$\endgroup\$ – Stefano Borini May 16 '11 at 18:30
  • \$\begingroup\$ Defining a class inside the method just serves to make things complicated and hard to read. Define the class outside of the method.. Agreed. I wanted to push the limit with that code in order to stress my understanding of closures and the general feeling. It's not production code, it's let's see what happens code. \$\endgroup\$ – Stefano Borini May 16 '11 at 18:32
  • \$\begingroup\$ Having return at the end of a function doesn't do anything and makes it look like you don't know what you are doing. That return was supposed to be a raise StopIteration, and I just didn't change it before posting. I wasn't concerned with that, but you are right that both (return or raising) are useless in that context. \$\endgroup\$ – Stefano Borini May 16 '11 at 18:34
  • \$\begingroup\$ Unless you really need to access the data row by row I suggesting flattening the iterator. I need to iterate by row because I render by row, and at each completed row I refresh the GUI. It's easier to roll over the rows, and inside that, roll over the columns. I may also flatten, roll over the pixels and refresh at each new row, but as I said, I'm stressing my knowledge for fun and profit. I wouldn't write that code in production, ever. \$\endgroup\$ – Stefano Borini May 16 '11 at 18:37
  • \$\begingroup\$ @Stefano Borini, why are you asking for a code review on push your limit code? The whole point of a code review is for helping make it production ready \$\endgroup\$ – Winston Ewert May 16 '11 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.