raysect / source

The main source repository for the Raysect project.
http://www.raysect.org
BSD 3-Clause "New" or "Revised" License
89 stars 23 forks source link

The wrong translation "pixel_to_local" in the Targetted_ccd? #391

Closed munechika-koyo closed 3 years ago

munechika-koyo commented 3 years ago

In the _generate_rays() function in TargettedCCDArray class, the way of translating ray origin points sampled by self._point_sampler might be wrong.

I think pixel_to_local should be defined as follows:

pixel_x = self._image_start_x - self._image_delta * (ix + 0.5) pixel_y = self._image_start_y - self._image_delta * (iy + 0.5) pixel_to_local = translate(pixel_x, pixel_y, 0)

I am sorry if my suggestion is wrong.

vsnever commented 3 years ago

Hi @munechika-koyo, I think your suggestion is correct. However, it is not just the TargettedCCDArray, it looks like CCDArray, PinholeCamera and OrthographicCamera are also affected. The RectangleSampler3D samples the points with the offset, namely in (-0.5 width, 0.5 width, -0.5 height, 0.5 height), not in (0, width, 0, height), so the origin point must be the pixel center, not the vertex.

CnlPepper commented 3 years ago

The RectangleSampler samples the rectangle defined by the ranges [-width / 2, width / 2] and [-height / 2, height / 2]. The pixel_x and pixel_y should be located at the centre of the pixels. Looking at the pinhole camera calculation, it does look like there is an error in the pixel positions. It is fortunately small for high pixel counts, but needs fixing. The small offset error is probably why we have not previously noticed it.

For the Pinhole camera:

if the image_max_width = 1.0 which means an image covering [-0.5, 0.5] and the number of pixels is (2, 2) then the centre of the pixels should be (-0.25, -0.25), (-0.25, 0.25), (0.25, -0.25) and (0.25, 0.25). The sampler width should be 0.5.

With the current implementation we get:

image_delta = 0.5    # pixel width
image_start_x = 0.5
image_start_y = 0.5

The pixel centres are then:

x=0, y=0: 0.5, 0.5
x=0, y=1: 0.5, 0.0
x=1, y=0: 0.0, 0.5
x=1, y=1: 0.0, 0.0

I suspect this error crept in a few years ago when Matt and I were rewriting the samplers. I'd need to check but I suspect the original sampling was [0, 1] rather than [-0.5, 0.5].

I'll commit a fix asap.

munechika-koyo commented 3 years ago

Thank you for your prompt response and fixing.