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

Performance improvements #80

Merged
merged 26 commits into from
Mar 19, 2019
Merged

Performance improvements #80

merged 26 commits into from
Mar 19, 2019

Conversation

joerg1985
Copy link

@joerg1985 joerg1985 commented Jan 10, 2019

Did some improvements to the collision check's and the padding.
This boosted the performance on my machine for CollisionMode.PIXEL_PERFECT.

Kind regards
Jörg

joerg1985 added 5 commits January 8, 2019 21:30
Depending on the width of the image a lot of int[] have been created, a BitSet with a size of width * height is a better choice here. It uses less memory and is faster than determinating a pixels transparency each time.
Create a copy of the original raster to do the shouldPad test and instant update the raster. This avoids a lot of object creation, hash calculation and garbage collection needed to maintain the HashSet.
It is more efficent to check the boudings outside the loop
should be a little faster
we expect the none transparent pixel to be around the center.
we check a 5 * 5 raster first to move faster to the center.
after this we still check all pixels to get a complete result.
@joerg1985 joerg1985 closed this Jan 12, 2019
@joerg1985
Copy link
Author

i found a bug, i will fix it and reopen the pull request

@kennycason
Copy link
Owner

@joerg1985 nice man! appreciate the work! Glad to see some performance improvement work! It's definitely needed.

@kennycason
Copy link
Owner

@joerg1985 I know I didn't have any tests before, but if you wouldn't mind adding a couple unit tests, that would also be super helpful for future work on this class. I also don't mind adding them after the fact since I know they were none existent to begin with. :)

@joerg1985 joerg1985 reopened this Jan 13, 2019
@joerg1985
Copy link
Author

I have done some further improvements and added a test for the collision detection.
The time running all tests change on my hardware from more than 1500s to 210s.
Could you confim this on your hardware?

joerg1985 added 3 commits January 14, 2019 17:49
@joerg1985
Copy link
Author

joerg1985 commented Jan 14, 2019

@kennycason I had a look at the unittests but it is hard to write them without good documentation of the classes. I will try add some unittests, but it might take some days.

@joerg1985 joerg1985 closed this Jan 14, 2019
@joerg1985 joerg1985 reopened this Jan 14, 2019
joerg1985 added 3 commits January 17, 2019 18:17
the concurrent creation of words could break client code
the graphics object used to create the font metrics had other RenderingHints than the graphics object to write the text, if the default values of the used jvm did not have the TEXT_ANTIALIASING set to TEXT_ANTIALIAS_LCD_HRGB.
@kennycason
Copy link
Owner

@joerg1985 woah! I like your speed ups you noticed. I will very happily try locally!

joerg1985 added 2 commits January 31, 2019 21:57
…o it

the maximum spiral radius might be bigger or smaller than the image width, e.g. if the image height is bigger than the width.
@kennycason
Copy link
Owner

@joerg1985 update on my end. I have recently left my job and will have more free-time to review all this. My goal is we get this in before end of next week. :)

this happened when the offset was 0
@joerg1985
Copy link
Author

joerg1985 commented Feb 3, 2019

This would be great, i think i am done now with the improvements. I think i fixed issue #60 in this pr, it was because of different rendering hints used by the graphics instances to meassure and to draw.

The rtree dependency could now easily removed, to reduce the footprint of the library, this might be good for andriod. The rectangle area of the placed word needs to be masked in the background and the rtree calls need to be removed.

@kennycason
Copy link
Owner

kennycason commented Mar 16, 2019

@joerg1985 I just pulled the branch down to verify locally and I see a couple test failures:

[ERROR] checkRotatedTextIsNotCropped[6](com.kennycason.kumo.RotateWordTest)  Time elapsed: 0.133 s  <<< FAILURE!
java.lang.AssertionError: text doesn't touch the top line
	at com.kennycason.kumo.RotateWordTest.checkRotatedTextIsNotCropped(RotateWordTest.java:81)

[ERROR] checkRotatedTextIsNotCropped[7](com.kennycason.kumo.RotateWordTest)  Time elapsed: 0.096 s  <<< FAILURE!
java.lang.AssertionError: text doesn't touch the top line
	at com.kennycason.kumo.RotateWordTest.checkRotatedTextIsNotCropped(RotateWordTest.java:81)

I'm also about to dig into them. I'd love to get this branch merged into the base.
I'll look into it a bit as well.

i'd love to remove unnecessary dependencies if possible as well

@kennycason
Copy link
Owner

@joerg1985 I have tested your branch with various integration tests and think it's great. It's definitely faster and the resulting images have a more compact, yet still aesthetically pleasing result. I'm tempted to @ignore the failing test and merge your work in since you've done so much already and then we can address the failed test (text touching the top line) later. I'll await your response for what you'd prefer.

@joerg1985
Copy link
Author

joerg1985 commented Mar 18, 2019

I added this test, because i was reworking the text rotation, but i disposed this and returned to the original implementation. I think you could ignore the test for now and after the changes have been merged we could have a deeper look in this. I think of might be related to different rendering behaviour of our systems, because the tests did not fail on my Windows 10, x64, openjdk 10.0.2, what ist your setup?

@kennycason
Copy link
Owner

@joerg1985 Sounds good! I suspected the same thing. I'll merge in, and fix @ignore the test then release. Thanks a lot! and super sorry for the massive delay on merging, I've had a high density of large life events. :)

@kennycason kennycason merged commit 3add8be into kennycason:master Mar 19, 2019
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.

2 participants