JohnSundell
Extract collision detection logic from Actor
To be able to add new types of SceneObjects that support collision detection without duplicating code, the collision detection logic in Actor needs to be extracted.
We probably need a new protocol something like this:
internal protocol Collidable {
var isCollisionDetectionEnabled: Bool { get set }
var rectForCollisionDetection: Rect { get }
}
Collision events also need to be extracted from ActorEventCollection.
Sounds great to me 👍 As for event collections, we can probably introduce a base class called CollidableEventCollection that any collidable object's event collection can inherit from.
I thought about the same thing, to add a CollidableEventCollection.
The only problem I see with this is what happens when we add a new protocol (e.g. Clickable) and want to move more events out of ActorEventCollection...
Would have been nice to add events as protocol extensions but then events can't be stored properties.
Could of course use objc_setAssociatedObject but I always tries to avoid that if I can.
@mattiashagstrand Imagine Engine actually already has a solution for that! 😄 You can simply call makeEvent() in an EventCollection and it will be stored automatically based on the function signature 👍 But that comes at a bit of a performance cost, which is completely fine for custom events that are not expected to be called multiple times per frame (potentially). But for the built-in events we want to use real Swift properties to enable quick static dispatch.
Nice! 😄
Still not sure how we should handle when we want to extract other types of events from Actor... 🤔
One option is to let Collidable have an associatedtype Events that needs to be both an EventCollection and conform to a CollidableEventCollection that defines the required events. That way the implementation will be duplicated, but the API will be consistent. Which I think is a nice tradeoff in this case, since it'll enable us to use composition 👍
I like this a lot! 👍 Absolutely agree that consistent API is more important than a little code duplication.
I'll start a new PR then?
@mattiashagstrand Do it 🚀 one thing to keep in mind when working with this part of the engine is that it's very performance sensitive code. We should probably invest in some performance tests going forward, but until then it would be awesome if you could measure your change against the current implementation before submitting a PR, to make sure we don't get a regression in performance. Let me know if you want some input on how to do this 🙂
Got it!
I was actually thinking about that yesterday that it would be nice to have some performance tests. There is something built in to XCTest but I have never used it. If you have any suggestions on the best way to do performance testing it would help a lot!
@mattiashagstrand Usually what I do is to create a scene with a large number of objects (say, 1000) and then use the Core Animation Profiler to make sure that everything still runs at 60 frames per second. I usually run those tests on an iPhone 6. Another thing I do (which is probably more appropriate for things like this), is to just run the code path 10,000 times or something and measure the time it takes (by just using a timestamp variable inline in the code).
I just created a test app that puts 1000 actors on the screen and they all have a rotate action:
actor.repeat(RotateAction(delta: 3.15, duration: 1))
When I profile on my iPhone 8 the CPU runs 100% and I only get around 35 FPS... 🤔
Oh snap 😱 I must admit I've never tried it with 1000 repeated rotate actions 😅 Let me run the same simulation here, would be interesting to see what it gets stuck on 🤔
@mattiashagstrand Did some investigations, and it turned out that collision detection was a large bottleneck when many objects are added close to each other (since it is turned on by default). I made a fix for that here: https://github.com/JohnSundell/ImagineEngine/pull/94. Will continue to look into how we can make 1000s of actions run smoothly at the same time - perhaps with some batching mechanism 😄 I want Imagine to always render at 60 FPS no matter what 🚀
@mattiashagstrand OK I've identified the problem. It's with Timeline, that when many actions are continuously updated it generates a TON of overhead (one of the top functions is Set.insert()). Will take a look soon and fix it 👍
Done! 😄 With this change even thousands of animations should now render smoothly (1000 simultaneous rotations now render at 60 FPS on my iPhone 7): https://github.com/JohnSundell/ImagineEngine/pull/95 🚀
I’ve done a fair amount of work with the built in performance testing stuff. It’s really nice! You give it a block of code to execute and it does so a bunch of times and measures it, reporting back average time and standard deviation, etc. And then it allows you to capture a baseline and reports gains/losses in performance vs that baseline each time it is run as part of the test suite. It can even be configured to fail tests when performance drops too much from the baseline.
If I get some time later I can take a look at adding some of these kinds of tests. Any particular chunks of the API you think I should focus on?
@aranasaurus Awesome! 😄 I added a new issue for performance testing here, along with some APIs that I think would be good to performance test: https://github.com/JohnSundell/ImagineEngine/issues/98. Let me know what you think 👍
@JohnSundell Excellent work with the performance improvements! 👏 Now I just have to make sure I don't make it worse again when I extract collision detection... 🙈
@mattiashagstrand Thanks 😄 Haha no worries, we'll keep measuring and will improve if needed 👌 This is why I wanted to make an open source Swift game engine in the first place, to be able to dive in and debug & fix performance problems whenever they occur.
Started looking at this and have defined a Collidable protocol like this:
internal protocol Collidable {
associatedtype Events: EventCollection<Self> where Events: CollidableEventCollection
var events: Events { get }
/// Whether the object is able to participate in collisions.
var isCollisionDetectionEnabled: Bool { get set }
/// Used to lazily activate collision detection when collisions
/// are observed.
var isCollisionDetectionActive: Bool { get set }
/// Rect used to determine if two objects are in contact
var rectForCollisionDetection: Rect { get }
}
And then I want to define CollidableEventCollection like this:
internal protocol CollidableEventCollection: class {
/// Event triggered when the object collided with another collidable object
func collided<C: Collidable>(with otherCollidable: C) -> Event<Self, C>
}
But if I have <C: Collidable> the compiler fails with a Segmentation fault: 11. It compiles if I just use <C>. @JohnSundell Have you seen this before?
@mattiashagstrand Yeah the problem is that you end up with a circular reference. The compiler wants to compile Events, but to do that it needs to compile Collidable, and then back to Events. Have seen these segfaults many times when trying to do things like this with generics. I think you can solve it by making CollidableEventCollection a subclass of EventCollection, but like we discussed before not sure if we want to go that route.
I suspected it had something to do with a circular reference. Would have been helpful with a error messages instead of segmentation fault... 🙄
Yeah, making CollidableEventCollection a class should be ok but isn't very flexible. Back to the drawing board... 🤔
@JohnSundell This is the closest I've gotten to a working solution.
Collidable is now defined like this:
/// Protocol adopted by objects that can collide with other objects
public protocol Collidable: class {
associatedtype Events = EventCollection<Self>
var events: Events { get }
/// Whether the object is able to participate in collisions.
var isCollisionDetectionEnabled: Bool { get set }
/// Used to lazily activate collision detection when collisions
/// are observed.
var isCollisionDetectionActive: Bool { get set }
/// Rect used to determine if two objects are in contact
var rectForCollisionDetection: Rect { get }
/// Any logical group that the object is a part of.
var group: Group? { get set }
}
and I have done this to add the collision events:
public extension EventCollection where Object: Collidable {
/// Event triggered when the object collided with another collidable object
func collided<C: Collidable>(with otherCollidable: C) -> Event<Object, C> {
object?.isCollisionDetectionActive = true
otherCollidable.isCollisionDetectionActive = true
return makeEvent(withSubject: otherCollidable)
}
/// Event triggered when the object collided with another collidable object in a given group
func collided<C: Collidable>(withCollidableInGroup group: Group) -> Event<Object, C> {
object?.isCollisionDetectionActive = true
return makeEvent(withSubjectIdentifier: group.identifier)
}
}
One downside to this solution is that Collidable has to be public and therefor variables like isCollisionDetectionActive are also public.
Another is that when you observe collisions for a group you have to specify the type of the subject:
actor.events.collided(withCollidableInGroup: group).observe { (_, _: Actor) in
}
I also noticed that collision detection and constraints handling are handled together and that this change requires changing quit a lot in Grid. What do you think is the best way forward with this? Have you thought of a better way to do this?