-
Notifications
You must be signed in to change notification settings - Fork 35
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
Memory/Cpu improvement #5
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch realising that we don't need to reload the images everytime. It should have a CPU impact for sure. Just a minor comments in order to keep it even simpler.
Thanks, great work!
@@ -368,7 +368,7 @@ - (CGPoint)pointInView:(UIView *)realityView forCoordinate:(ARGeoCoordinate *)co | |||
} | |||
|
|||
- (void)updateLocations:(NSTimer *)timer { | |||
[radar updatePoints:centerCoordinate]; | |||
[radar updatePoints:centerCoordinate reloadData:NO]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is never invoked with YES
then, I'd remove the new argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reloaddata wont be called here, but if you want to call it manually you could call it somewhere else from superview on data changes without reallocating everything.
} | ||
|
||
// Only remove subviews if reloading the data with new points | ||
if (reloadData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data will never be reloaded so remove this bit. Instead I'd check somehow that if this is the first call (i.e., no dots in the dotsView
), then dot views have to be created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point was for compatibility if you want to invoke reloaddata from superview without reallocating the entire radarview from beginning, for example you change positions and want to change the number of points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is needed at some point then it will be added. For now I'd rather keep it the simplest. :)
UIImageView *point; | ||
|
||
// Only create new imageviews if reloaddata or initial load | ||
if (points.count != dotsView.subviews.count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All dot views will already be created (from line 40), so we just need to point = [dotsView.subviews objectAtIndex:count];
here.
point = [dotsView.subviews objectAtIndex:count]; | ||
} | ||
|
||
point.tintColor = [UIColor redColor]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why red?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to clean that part , it was more of an indicator if you want to change the standard color of the image.
Hi @Sneakr how are you doing with this? Do you need any help? |
@calonso hey, I asked what to do to get this merged , maybe you miseed that message, this is my first pull request if you can point me in direction I can finish it of course, not sure what to do from here lol! |
Ok, so the way this works is that normally you go through my comments fixing them, commit and push changes again, I review and make comments and so on until we get to a point where we are both happy with the final result and we merge it. Hopfeully it won't take very long to get to this point as your changes are quite good already and my comments are just minor ones. |
Hey @Sneakr how are you doing? Do you need any help with this? |
Hey man, Sorry for the delay im not very active, This github interface really makes me dizzy, my I get brainfreeze from everything here, you are free to just copy the edits and make the improvments as you wish and merge it! |
That's a pity man, we were so close, just a couple of minor comments... |
Changed bad recreation of UIImageViews on every movement frequency cycle and saves performance.