V86E: 'WritePixels' doesn't offset properly
Monkey Forums/Monkey Bug Reports/V86E: 'WritePixels' doesn't offset properly
| ||
| The following issue affects V86E (Latest) and its version of Mojo. EDIT: I decided to make a pull-request with the best of both worlds. Following the discussion here, I realized that 'WritePixels' doesn't behave with 'CreateImage'. This is because 'WritePixels' uses inadequate bounds checks in debug mode. Basically, it checks if the parameters match the size the image represents, but not its actual position/area. This means 'WritePixels' causes an error, even when the arguments were correct. This means 'WritePixels' needs to be relative to the area of the surface the image takes up. Well, that or you offset the input the other way. The problem is, this doesn't offset the user's input in either direction, making it incorrect. The two solutions are: A) Add the image's offset to 'x' and 'y' at the end of the function. This would translate x and y from the input (Relative in this case) to the intended place on the surface. For this method, this should only be done when the native call to 'WritePixels2' is made. You'd probably need to get the position offset using the first 'Frame' object. B) Consider 'x' and 'y' to be global, then perform a full bounds check based on the number of frames or size of the surface. I've written an example of this behavior. Execute that example and you'll find that it works initially, using both debug and release modes in fact. Now try disabling 'WRITEPIXELS_DEMO_HACK' at the top of the file, then try a debug build. This will emit an incorrect error following the current rules, that is. The time you originally ran it, it was using only 1 image with 'CreateImage', then writing the frames as intended. The second time around, it's using areas of the surface, meaning the positions are global. In release mode, the second form works perfectly, since it's already valid, but not in debug. This illustrates why the error checking is flawed. Now, let's make the assumption each call to 'WritePixels' is image-relative, rather than surface-based. This would be option A. You can then uncomment both preprocessor variables. This means the writes are all at 0 on both axes. This does not work right now. If this was the intention, as it is with 'GrabImage' already, then option A should be followed. But then again, maybe the best option is a boolean for relative vs. global coordinates? Whatever the case, this nonsense is exactly why I dislike Mojo 1's frame system. I'm definitely glad you changed it in Mojo 2. Mojo 1 is still used, though, and this is a bug with it. The proposed changes should be done to this method. |
| ||
| Not sure about the need for 'relative' - can't they just write to the 'parent' image? If relative is always on, then error checking looks OK to me - except that I forgot to add frame[n].x and frame[n].y for the final write. Will fix. |
| ||
| The reason I proposed 'relative' was because 'CreateImage' is limited by 'WritePixels'. Basically, 'CreateImage' can have multiple frames of the described size, but 'WritePixels' doesn't account for the other frames' involvement in its sanity checks. These checks are only done in debug mode, so otherwise valid code fails in release mode. I'm proposing that by default, you have the current behavior, only with it offset properly. However, I'm also saying that some people may be using this "incorrectly" (Global positions) if they chose multiple frames with 'CreateImage', so it's better to just give people the option. With 'relative' disabled, you're treating it as a direct write to the surface. This means 'x' and 'y' are global to everything using the surface. Likewise, the width and height of the surface would be used when 'relative' is disabled. This gives proper control over the allocated frames in the 'CreateImage' example. With 'relative' enabled, you are dealing with coordinates offset by where on the surface the image was grabbed from. With 'CreateImage', this is obviously position 0x0. However, if the operation's relative, we need to perform a proper bounds check under debug mode. In this case, we have two options: A) Restrict the area on the surface to a single "frame". (The first frame representing the parameter passed to 'CreateImage'. B) Restrict the area of the surface to multiple frames. Of course, unlike when 'relative' is disabled, these would both start at a relative position on the surface. The problem here is that limiting the user to the first frame's bounds makes it impossible to use 'CreateImage' with multiple frames. So, the choices are the 'relative' argument, or checking all frames' bounds. Sorry for the late reply, it was bad timing on my part. |
| ||
| > The problem here is that limiting the user to the first frame's bounds makes it impossible to use 'CreateImage' with multiple frames. Ok, think I've got you now. How about this - it simply adds a 'frame' param (which is another bug report/request): Method WritePixels( pixels[],x,y,width,height,offset=0,pitch=0,frame=0 ) If Not pitch pitch=width #If CONFIG="debug" If frame>=frames.Length Error "Invalid frame" Local w:=Self.width If flags & XPadding w+=2 Local h:=Self.height If flags & YPadding h+=2 If x<0 Or y<0 Or x+width>w Or y+height>h Error "Invalid pixel rectangle" If offset<0 Or pitch<0 Or offset+(height-1)*pitch+width>pixels.Length Error "Invalid array rectangle" #End device.WritePixels2 surface,pixels,frames[frame].x+x,frames[frame].y+y,width,height,offset,pitch End (wish I'd never put that padding crap in there...) Allowing access to the 'parent' rect wont always work because it's not always a rect - frames can 'wrap around' if they don't fit on a single row (if anyone's crazy enough to do this!). It's also possible to 'GrabImage' from within a grabbed image I think... |
| ||
| Sorry, I had something come up with my family. This looks good to me. Everything checks out with the unit-test I made. Go ahead and push it when you get the chance. |
   |