Screen edge offscreen fix. #20968

New Issue

Anthony Edlin · 2010-02-04T18:52:18+01:00

Anthony Edlin commented

2010-02-04 18:52:18 +01:00

%%%The area and region rects that get calculated are 1 too big for regions and areas along the top and right side of the screen. For instance on a moniter 1680x1050 and a fullscreen window with an area or region spanning the whole window width, it's rects would report a size of 1681. This is due to the screen verts being placed outside the window by the screen_test_scale function. Note that the bottom and left screen verts are always placed inside the window.

There's a couple of options to fix this inconsistency, change the bottom and left verts to also be 1 outside the window and then change how the area rects are calculated, or make the screen verts always be inside the window. This patch chooses the later because I had more weird problems with the other way.

Once screen_test_scale() was changed to keep the verts inside the window the area and region rects were correct, but now there was an extra line at the top of the screen, probably because someone had to make the min y size of areas 27 or the top header would hide itself in some circumstances. So I changed the AREAMINY back to 26. I also now had to make the screen edge move operator set the correct limits based on if its an interior edge or an edge along the outside. The header was still hiding though at a height of 26 so I traced that back to another off-by-one error in the rct_fits() function.

One last thing this patch fixes is that you could select the screen edges along the side of the window (Note: this does happen in regular blender along the bottom and left edges only) but you couldn't move them. So I changed the screen_find_active_scredge() function to not return edges along the window for now.%%%

%%%The area and region rects that get calculated are 1 too big for regions and areas along the top and right side of the screen. For instance on a moniter 1680x1050 and a fullscreen window with an area or region spanning the whole window width, it's rects would report a size of 1681. This is due to the screen verts being placed outside the window by the screen_test_scale function. Note that the bottom and left screen verts are always placed inside the window. There's a couple of options to fix this inconsistency, change the bottom and left verts to also be 1 outside the window and then change how the area rects are calculated, or make the screen verts always be inside the window. This patch chooses the later because I had more weird problems with the other way. Once screen_test_scale() was changed to keep the verts inside the window the area and region rects were correct, but now there was an extra line at the top of the screen, probably because someone had to make the min y size of areas 27 or the top header would hide itself in some circumstances. So I changed the AREAMINY back to 26. I also now had to make the screen edge move operator set the correct limits based on if its an interior edge or an edge along the outside. The header was still hiding though at a height of 26 so I traced that back to another off-by-one error in the rct_fits() function. One last thing this patch fixes is that you could select the screen edges along the side of the window (Note: this does happen in regular blender along the bottom and left edges only) but you couldn't move them. So I changed the screen_find_active_scredge() function to not return edges along the window for now.%%%

Anthony Edlin commented

2010-02-04 18:52:18 +01:00

Changed status to: 'Open'

Anthony Edlin commented

2010-02-17 20:05:30 +01:00

%%%Added some images to better show what this patch does.%%%

Anthony Edlin commented

2010-02-17 20:14:40 +01:00

%%%btw, I switched around the colors for the pictures so it can be seen easier. Green=region top and right emboss, Blue=region bottom and left emboss, Red=screen area edge.%%%

Anthony Edlin commented

2010-02-17 21:45:34 +01:00

%%%Added picture from XP to show it's not just in linux, emboss and screen edge colors aren't changed like I did for the other pictures but you can still see that there is no emboss on the right side of the 3dview that's along a window edge because of the screen edge being outside the window.%%%

Ton Roosendaal commented

2010-02-21 17:22:07 +01:00

%%%Will investigate asap%%%

Anthony Edlin commented

2010-02-28 18:29:54 +01:00

%%%I know reviewers are busy right now, I can add more description explaining each line of the patch if it'll help, and I can make/discuss any changes to the patch if required, let me know.%%%

Campbell Barton commented

2010-03-09 16:16:51 +01:00

%%%Hi, I applied the patch and noticed the view3d was offset compared to the view2d.
Notice the blue line to the right in the view3d and header are not aligned.

attached image. keeping assigned to Ton.%%%

%%%Hi, I applied the patch and noticed the view3d was offset compared to the view2d. Notice the blue line to the right in the view3d and header are not aligned. attached image. keeping assigned to Ton.%%%

Anthony Edlin commented

2010-03-09 19:16:37 +01:00

%%%Hi Campbell,

That was not changed by this patch. That is just how the view3d's emboss is drawn compared to view2d's emboss. If you look at the attached NOT patched image you can see the view3d and view2d are drawn as you describe. I just assumed this was how they were supposed to be so I didn't change anything related to that. Is this difference between the emboss drawing not intended?

To reiterate the main points of this patch.

Fixes an off-by-one error in screen_test_scale() which causes the areas and regions to draw 1 bigger on the right and top side of the window, therefor hiding one line of pixels.
Fixes an off-by-one error in rct_fits() which causes regions to incorrectly hide even though it would fit inside the area.
Correctly set the limits for the screen edge move operator so it will always go up to AREAMINX and AREAMINY.
Change screen_find_active_scredge() so it doesn't show the arrows cursor on the screen edges along the window border.%%%

%%%Hi Campbell, That was not changed by this patch. That is just how the view3d's emboss is drawn compared to view2d's emboss. If you look at the attached NOT patched image you can see the view3d and view2d are drawn as you describe. I just assumed this was how they were supposed to be so I didn't change anything related to that. Is this difference between the emboss drawing not intended? To reiterate the main points of this patch. 1) Fixes an off-by-one error in screen_test_scale() which causes the areas and regions to draw 1 bigger on the right and top side of the window, therefor hiding one line of pixels. 2) Fixes an off-by-one error in rct_fits() which causes regions to incorrectly hide even though it would fit inside the area. 3) Correctly set the limits for the screen edge move operator so it will always go up to AREAMINX and AREAMINY. 4) Change screen_find_active_scredge() so it doesn't show the arrows cursor on the screen edges along the window border.%%%

Anthony Edlin commented

2010-05-04 19:41:47 +02:00

%%%Hi, I think all of these off-by-ones are still there, and I just tested this patch again today and it still seems to work on svn revision 28574.

Thanks.%%%

%%%Hi, I think all of these off-by-ones are still there, and I just tested this patch again today and it still seems to work on svn revision 28574. Thanks.%%%

Anthony Edlin commented

2010-08-10 21:42:35 +02:00

%%%Hi, I think all of these issues are still present, but this patch doesn't apply cleanly anymore. It's a relatively simple fix if you just look at the diffs and understand the code. I'm dropping maintaining this patch in my personal branch as it'll just get worse in the future and I'm not sure it'll be applied, but it will be here in tracker for someone if it's needed.

Thanks.%%%

%%%Hi, I think all of these issues are still present, but this patch doesn't apply cleanly anymore. It's a relatively simple fix if you just look at the diffs and understand the code. I'm dropping maintaining this patch in my personal branch as it'll just get worse in the future and I'm not sure it'll be applied, but it will be here in tracker for someone if it's needed. Thanks.%%%

Ton Roosendaal commented

2010-08-11 11:38:35 +02:00

%%%Hi Anthony,

I'm terribly sorry we can't pick up these patches in a timely manner. It's on the agenda for sure, but the priorities for me are still elsewhere :)

Thanks,

Ton-%%%

%%%Hi Anthony, I'm terribly sorry we can't pick up these patches in a timely manner. It's on the agenda for sure, but the priorities for me are still elsewhere :) Thanks, - Ton-%%%

Anthony Edlin commented

2012-10-27 21:58:14 +02:00

%%%I'm pretty sure all of these issues are still present after 2+ years, so I've updated this old patch for a recent revision. It's attached as the -v2.patch

I know this whole area is not very intuitive, and the whole screen vert/edge thing should be re-factored completely to remove the need for most of these changes and make it easier to understand. I also know that this probably isn't very important because to my knowledge no one has ever reported these issues in the bug tracker. But it still just bothers me that all these off-by-one errors are still there in the code.

If anyone has any questions about the patch, I'm very willing to discuss/explain anything about it.%%%

%%%I'm pretty sure all of these issues are still present after 2+ years, so I've updated this old patch for a recent revision. It's attached as the -v2.patch I know this whole area is not very intuitive, and the whole screen vert/edge thing should be re-factored completely to remove the need for most of these changes and make it easier to understand. I also know that this probably isn't very important because to my knowledge no one has ever reported these issues in the bug tracker. But it still just bothers me that all these off-by-one errors are still there in the code. If anyone has any questions about the patch, I'm very willing to discuss/explain anything about it.%%%

Ton Roosendaal commented

2012-10-29 10:03:56 +01:00

%%%Thanks, I'm indeed back coding :) The amount of changes in the patch require more attention though - i'll check on it very soon.%%%

Anthony Edlin commented

2012-10-29 18:43:56 +01:00

%%%Would it help if I put the patch up on codereview? I could explain every line of the patch then if needed.%%%

Ton Roosendaal commented

2012-12-08 18:04:18 +01:00

%%%I'm just back from a 3 week holidays - needed a real break. I am not forgetting this, but there's a lot on the todo here. I will come back to it, promised :)%%%

Anthony Edlin commented

2013-05-02 08:45:13 +02:00

%%%The pixelsize additions to trunk broke this patch into a million pieces, yay.%%%

Anthony Edlin commented

2013-05-02 10:15:08 +02:00

%%%Well I updated the patch with a v3, but I'm really not sure about all the pixelsize changes as that stuff is confusing where it starts and ends in the code, and I can't test them at all as I don't have the hardware or desire to fiddle around with it. I did build it and it worked as expected for me.%%%

Anthony Edlin commented

2013-10-18 04:21:30 +02:00

%%%:(%%%

Anthony Edlin commented

2013-10-20 17:49:33 +02:00

%%%):%%%

Ton Roosendaal commented

2013-10-20 18:22:54 +02:00

%%%Sorry for leaving you so long without reply.

I have to acknowledge that my available time for coding is way below a responsible maintainer's level - so I have to ask someone else to handle it.
Assigned to Brecht now.
%%%

%%%Sorry for leaving you so long without reply. I have to acknowledge that my available time for coding is way below a responsible maintainer's level - so I have to ask someone else to handle it. Assigned to Brecht now. %%%

Ton Roosendaal commented

2013-10-20 18:22:54 +02:00

Changed status from 'Open' to: 'Resolved'

Ton Roosendaal closed this issue

2013-10-20 18:22:54 +02:00

Anthony Edlin commented

2013-11-21 22:47:46 +01:00

So, these problems are still in the region and screen code. It's not that hard to understand. These aren't sweeping UI changes. They just try to make the code that is there do the correct thing.

Back when I created this patch I had plans to continue with some changes in how scaling works to consider the area type and if it was worth scaling up (such as properties editor or info editor wouldn't need to scale up if it was already a certain size), but after the lack of action on this patch I realized it wasn't worth it to continue working in this area of blender at the time. I mean, this patch is essentially fixing some beginning coder off-by-one errors, and if I can't get these things fixed what are the chances of something slightly more complicated being reviewed.

I think after over three and a half years of repeatedly updating this relatively simple patch and then having it broken from trunk before it's reviewed despite comments saying it will be looked at has left me pretty frustrated. I realize my recent comments (including this comment) on this patch aren't very helpful and slightly passive aggressive, so I apologize for them. I'll try to reset my brain on this and just start over.

So with that said I'll take some time in the next couple of days and go over this patch again with the new git repo (really happy with the new git migration) and double check that it works with the pixalsize changes and applies cleanly.

So, these problems are still in the region and screen code. It's not that hard to understand. These aren't sweeping UI changes. They just try to make the code that is there do the correct thing. Back when I created this patch I had plans to continue with some changes in how scaling works to consider the area type and if it was worth scaling up (such as properties editor or info editor wouldn't need to scale up if it was already a certain size), but after the lack of action on this patch I realized it wasn't worth it to continue working in this area of blender at the time. I mean, this patch is essentially fixing some beginning coder off-by-one errors, and if I can't get these things fixed what are the chances of something slightly more complicated being reviewed. I think after over three and a half years of repeatedly updating this relatively simple patch and then having it broken from trunk before it's reviewed despite comments saying it will be looked at has left me pretty frustrated. I realize my recent comments (including this comment) on this patch aren't very helpful and slightly passive aggressive, so I apologize for them. I'll try to reset my brain on this and just start over. So with that said I'll take some time in the next couple of days and go over this patch again with the new git repo (really happy with the new git migration) and double check that it works with the pixalsize changes and applies cleanly.

Anthony Edlin commented

2013-11-21 22:50:00 +01:00

Changed status from 'Resolved' to: 'Open'

Anthony Edlin reopened this issue

2013-11-21 22:50:00 +01:00

Anthony Edlin commented

2013-11-21 22:50:00 +01:00

I see Ton closed this task as Resolved, which I assume was an error in assigning it to brecht, so I'm opening it again.

Thanks.

I see Ton closed this task as Resolved, which I assume was an error in assigning it to brecht, so I'm opening it again. Thanks.

Brecht Van Lommel commented

2013-11-21 23:37:47 +01:00

Looks like a mistake indeed.

When you have a patch that applies on the latest master you can submit it straight to code review and assign me as a reviewer.

Looks like a mistake indeed. When you have a patch that applies on the latest master you can submit it straight to code review and assign me as a reviewer.

Anthony Edlin commented

2013-11-25 05:56:38 +01:00

I've uploaded a v4 for this patch. I'll add to differential too, which is I assume what you meant when you said code review. I don't have hardware to test the pixelsize stuff myself but I hacked it larger in linux ghost code and tested and it seemed to do the correct stuff, still someone to test on correct hardware would be good I think.

Thanks.

I've uploaded a v4 for this patch. I'll add to differential too, which is I assume what you meant when you said code review. I don't have hardware to test the pixelsize stuff myself but I hacked it larger in linux ghost code and tested and it seemed to do the correct stuff, still someone to test on correct hardware would be good I think. Thanks.

Brecht Van Lommel commented

2013-11-25 06:09:10 +01:00

Differential is indeed what I meant. I have a retina laptop here so can test the pixelsize.