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

Wip core #199

Merged
merged 6 commits into from
May 25, 2012
Merged

Wip core #199

merged 6 commits into from
May 25, 2012

Conversation

dmose
Copy link
Member

@dmose dmose commented May 24, 2012

Here are a bunch of spaces test dperit and I worked out, some paired, some not. One of them currently fails in an odd way, more on that in a bit.

I moved the spaces constructor next to the prototype in the spaces.js file, as that's standard style and where folks would generally expect to look.

Questions that came up while writing these tests:

The engine has its own RealTimeClock. Is this likely to be as accurate as date.now() or the new high-perf timers? Does that matter?

For the tests, we ended up reaching inside Entity somewhat. It wasn't clear to us why entity._children was a private member. Is there some reason components people shouldn't be able to inspect it?

It's currently possible to create a completely empty entity by supplying no arguments to the constructor. Is there a use case for this, or should it throw?

find(All)With is a little terse as far as names go. Maybe findWithComponentType?

Would it make sense for add() and remove() to return the values they were passed to make for easy chaining?

Right now, removing an entity that's not in a space doesn't do anything or notify the caller. Should it (for example) throw?

These tests nail down a deterministic return order for the various find and findAll (items should be returned in the order added), in part because those were the easier tests to write, but also under the theory that a deterministic API is likely to be less surprising to people who read docs closely (most people).

Finally, doing new Space() without arguments works in the examples which include the engine, because by some black magic, the space constructor ends up with a clock local variable that's non-empty. dperit and i don't see how this is possible. The unit test for that, however, fails, because the code behaves the way we would expect from reading it. Insight here would be much appreciated.

From reading the comments in the Space constructor(), that unit test should actually be testing not just the the clock is an instanceof Clock, but probably that it's an instance of the system clock. Would you agree?
If so, it's not entirely clear how one would test that without depending on the engine. Thoughts?

@ghost ghost assigned modeswitch May 24, 2012
@modeswitch
Copy link

I moved the spaces constructor next to the prototype in the spaces.js file, as that's standard style and where folks would generally expect to look.

Agreed, but I'd rather defer this since all other modules follow the other pattern. If you open an issue for this I'll fix everything in one pass.

The engine has its own RealTimeClock. Is this likely to be as accurate as date.now() or the new high-perf timers? Does that matter?

It does matter. I would prefer to use HRT but again, separate issue for this.

For the tests, we ended up reaching inside Entity somewhat. It wasn't clear to us why entity._children was a private member. Is there some reason components people shouldn't be able to inspect it?

Adding methods (like the ones in Space) is a better solution. Shouldn't need to access children directly.

It's currently possible to create a completely empty entity by supplying no arguments to the constructor. Is there a use case for this, or should it throw?

Possibly. Need to think about this.

find(All)With is a little terse as far as names go. Maybe findWithComponentType?

Too verbose. The terse name might be confusing for new people, but that's not the case we should optimize for.

Would it make sense for add() and remove() to return the values they were passed to make for easy chaining?

Defnitely.

Right now, removing an entity that's not in a space doesn't do anything or notify the caller. Should it (for example) throw?

Should throw in debug mode. But we don't have a debug mode yet.

These tests nail down a deterministic return order for the various find and findAll (items should be returned in the order added), in part because those were the easier tests to write, but also under the theory that a deterministic API is likely to be less surprising to people who read docs closely (most people).

This is a stronger constraint than I had intended, but I suppose deterministic is easier to use.

Finally, doing new Space() without arguments works in the examples which include the engine, because by some black magic, the space constructor ends up with a clock local variable that's non-empty. dperit and i don't see how this is possible. The unit test for that, however, fails, because the code behaves the way we would expect from reading it. Insight here would be much appreciated.

The engine exposes the Space constructor bound to its clock:

simulation.Space.bind( null, this.simulationClock );

This will probably need to change to allow for realtime spaces.

From reading the comments in the Space constructor(), that unit test should actually be testing not just the the clock is an instanceof Clock, but probably that it's an instance of the system clock. Would you agree?
If so, it's not entirely clear how one would test that without depending on the engine. Thoughts?

Doesn't necessarily need to be the system clock. You can make a space with whatever clock you want. The space constructors on the engine instance should be bound to the system clocks, and we should test that in the engine unit tests.

@modeswitch modeswitch merged commit 37d0c72 into gladiusjs:wip-core May 25, 2012
@dmose
Copy link
Member Author

dmose commented May 25, 2012

I moved the spaces constructor next to the prototype in the spaces.js file, as that's standard style and where folks would generally expect to look.

Agreed, but I'd rather defer this since all other modules follow the other pattern. If you open an issue for this I'll fix everything in one pass.

OK, reverted, and issue #202 filed.

The engine has its own RealTimeClock. Is this likely to be as accurate as date.now() or the new high-perf timers? Does that matter?

It does matter. I would prefer to use HRT but again, separate issue for this.

Filed as issue #203.

For the tests, we ended up reaching inside Entity somewhat. It wasn't clear to us why entity._children was a private member. Is there some reason components people shouldn't be able to inspect it?

Adding methods (like the ones in Space) is a better solution. Shouldn't need to access children directly.

As per hangout discussion, got rid of unnecessary tests groping inside of the Space object.

It's currently possible to create a completely empty entity by supplying no arguments to the constructor. Is there a use case for this, or should it throw?

Possibly. Need to think about this.

As per hangout discussion, this is theorized to be useful for the case where people want to make an arena of reusable entity objects. Leaving this alone.

Would it make sense for add() and remove() to return the values they were passed to make for easy chaining?

Defnitely.

Issue #204 filed.

Right now, removing an entity that's not in a space doesn't do anything or notify the caller. Should it (for example) throw?

Should throw in debug mode. But we don't have a debug mode yet.

As per our hangout discussion, I've made it throw now, and adjusted the test.

Finally, I've moved the new Space() test over to engine.test.js, and I've filed issue #205 about sorting out the various Space/Clock API framing bits that need love.

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