Improve jump arrows drawing

eteran / edb-debugger

edb is a cross-platform AArch32/x86/x86-64 debugger.

GNU General Public License v2.0

2.7k stars 326 forks source link

Improve jump arrows drawing #726

Closed shahrilnet closed 5 years ago

shahrilnet commented 5 years ago

Previously, some jump arrows might be overlapped or being so close together (which makes it not to look nice).

This PR attempted to fix that, and now all jump arrows fit nicely. :)

eteran commented 5 years ago

Does this take into account my refactor that I push a little bit ago?

shahrilnet commented 5 years ago

Yes, I've merged your commits (little while ago) with mine.

eteran commented 5 years ago

OK, cool. I'll review it in a little bit and give @10110111 a chance to comment too! :-)

Thanks again, this stuff is looking great.

shahrilnet commented 5 years ago

You're welcome, happy to contribute a little bit. :)

10110111 commented 5 years ago

Some comments on the whole changeset including the previous (merged) one follow.

Enabling antialiasing results in the vertical separator lines being blurred into two pixels instead of being exactly 1-pixel wide. This is not good. (I'd actually prefer the arrows to also be 1-pixel wide, but that's up to you and @eteran to decide.)
If a jump targets into the middle of an instruction, the arrow line is drawn to the bottom of the viewport, missing the line containing the target byte.

The following screenshot demonstrates both above mentioned problems: Screenshot - 251019 - 00:03:39

The arrows in the following screenshot look confusing:
The arrows for untaken branches are asymmetric with respect to their path line:
The following display is misleading: I've actually come to this state by stepping from 0xf7fd7817 (skipping the conditional jump), but the red arrow hints as if I have just jumped here from 0xf7fd77fb. Maybe the arrow from the origin shouldn't be painted red.

shahrilnet commented 5 years ago

Arrow there is red highlighted because there is a direct jump from 0xf7fd77fb, the reason why I do highlight it is to give a clarification that this jump to this address is always taken

Edit: Fixed. Now arrow will not be red highlighted if arrow destination is EIP

I've fixed this by adding more horizontal length to one of the jump blocks if it is overlap at the edge.

Fixed where jump now points to the middle of instruction (see screenshot above)

I have removed anti-aliasing code. If @eteran has other to say I will add it back.

Thank you for all the comments, really appreciated it. :)

10110111 commented 5 years ago

I have removed anti-aliasing code.

I think the arrow tips could benefit from antialiasing. It just has to be enabled in QPainter right before that code and disabled afterwards. Just be sure to make them still look symmetric (note that in antialiased case the samples are offset by 0.5 px).

shahrilnet commented 5 years ago

@10110111

As per your suggestion, I've enabled anti-aliasing when only it is time to draw arrows. Arrow tips also now drawn in full area (using QPainter's fillpath)

10110111 commented 5 years ago

I'm OK with these changes now. I was thinking of some pixel-hunting tweaks, but I suppose I'll simply make a follow-up PR when I finalize my ideas.

eteran commented 5 years ago

@10110111 's review has proven to be very worth having! Thanks to both of you. This PR looks great to me. I'll probably merge later today.