Skip to content

Conversation

@fedirnp
Copy link

@fedirnp fedirnp commented Sep 5, 2014

v.1.1 of text direction feature detector :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@dchaplinsky
Copy link
Owner

Generally looks nice.
Please fix typos, remove unused code and run it through python linter.

Thank you for all your hard work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

degrees.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only include into histograms for pixel[0]<MAGIC_COLOR_THRESHOLD (10) values 255-pixel[0]. So the minimum value in histogram would be 245, where it's not 0. I suggest replacing this check with "if value != 0:" or just "if value:" to make the code easier to follow

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is pretty dirty. Planned to remove that code and add image binarization in the beginning...

@fedirnp
Copy link
Author

fedirnp commented Sep 13, 2014

Guys, take a look into the latest version. :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous request still holds: please describe what is each of these "magic" values

@parabolala
Copy link
Contributor

By the way, in case of just a single line of text, your approach seems to mistake letters for separate lines, i.e. the answer is wrong by 90º.

@fedirnp
Copy link
Author

fedirnp commented Oct 16, 2014

Guys, I've fixed code accoring your comments.
Please check it out!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove MAGIC_ from here and other constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants