Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate graph layout code from GraphView. #1414

Merged
merged 3 commits into from Apr 4, 2019

Conversation

Projects
None yet
3 participants
@karliss
Copy link
Contributor

karliss commented Mar 31, 2019

Goal of these changes is to simplify development and reusability of graph layout code.

  • Easier to work on two smaller and simpler modules
  • Allow experimenting with alternative layout algorithms. Multiple of them can be used if it turns out that there is trade off between performance and readability.

This PR should contain only code refactoring, any changes in behavior are unintentional.

Test plan (required)

  • Open an executable with some medium to big functions using this version and master
  • Visually compare the output, it should be identical

Closing issues
None

@karliss

This comment has been minimized.

Copy link
Contributor Author

karliss commented Apr 1, 2019

Planned changes are mostly implemented. While doing the testing I found some changes in resulting image, need to investigate if they are caused by my changes, slightly different cutter/r2 version, original code isn't deterministic.

@karliss karliss marked this pull request as ready for review Apr 1, 2019

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Apr 2, 2019

This is a great idea, love it! Will find the time to dig into it soon :)

@ITAYC0HEN

This comment has been minimized.

Copy link
Member

ITAYC0HEN commented Apr 2, 2019

While doing the testing I found some changes resulting image

I don't understand what did you mean?
btw, do you have telegram account? if so, please ping me there @megabeets

@karliss

This comment has been minimized.

Copy link
Contributor Author

karliss commented Apr 2, 2019

Regression: edges in graph overview don't have colors.

EDIT: fixed in 1a2891e

@karliss

This comment has been minimized.

Copy link
Contributor Author

karliss commented Apr 2, 2019

While doing the testing I found some changes resulting image

I don't understand what did you mean?
btw, do you have telegram account? if so, please ping me there @megabeets

There were some minor differences in layout compared to .appimage version, but it turned out that was caused by differences in r2 version not my changes.

@karliss karliss force-pushed the karliss:graph-layout-refact branch from 1a2891e to 8ba2b04 Apr 3, 2019

@karliss

This comment has been minimized.

Copy link
Contributor Author

karliss commented Apr 3, 2019

Rebased to resolve merge conflict.

@xarkes

xarkes approved these changes Apr 3, 2019

Copy link
Member

xarkes left a comment

Good work, thank you!

@ITAYC0HEN ITAYC0HEN merged commit cb51496 into radareorg:master Apr 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.