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

Try using array table for cell items #30

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ax-jason
Copy link

If the option using_orderedQuery is set to true, bump will use the predictable arrary for internal items query operation, which provide more precise simulation results. By default, it's set to false.

If the option using_orderedQuery is set to true, bump will use the predictable arrary for internal items query operation, which provide more precise simulation results. By default, it's set to false.
@ax-jason ax-jason changed the title Add using_orderedQuery option on newWorld Add using_orderedQuery option to newWorld Nov 11, 2016
@coveralls
Copy link

coveralls commented Nov 11, 2016

Coverage Status

Coverage decreased (-2.4%) to 95.185% when pulling a3f0313 on ax-jason:master into 4d776be on kikito:master.

@kikito
Copy link
Owner

kikito commented Nov 11, 2016

Hi there,

This is an interesting approach. I had not thought about using both a dictionary and an array, "switching the last item before erasing it" as you do here. I have only browsed the code very briefly and I still don't understand exactly how it works, but the tests pass, so it's working. I think this is worth exploring further. I have some comments:

  • Remove the using_orderedQuery option. If we're doing this, we're doing it full on - using arrays all the time. Also, remove any code which is not used any more as a result (for example, I assume the functiongetDictItemsInCellRect won't be needed).
  • You have a constant typo in several places. You have written "Arrary" but it's "Array" (without an "r" before the "y")
  • There is some inconsistency in your use of camelCaseNames vs underscore_names. In bump, the only underscored names are to differentiate "internal libraries" (like rect_ and grid_). Everything else is camelCase. For example, you should replace weak_value_mt by weakValueMt.
  • In the code you are using two different collections of items - world.items is a "dictionary", while world.items_array (which I hope you rename to world.itemsArray) is an array. I think it would be more consistent if world.items was renamed to world.itemsDictionary then. Similarly, each cell's collection of items should be renamed from cell.items to cell.itemsDictionary.

Regards!

Remove the using_orderedQuery option.
Resolve camelCaseNames/underscore_names problem.
Fix some mistakes.
@coveralls
Copy link

coveralls commented Nov 11, 2016

Coverage Status

Coverage increased (+0.03%) to 97.598% when pulling dc73746 on ax-jason:master into 4d776be on kikito:master.

@ax-jason
Copy link
Author

All done.
And the switching thing is like this:
When removing a item, there will be a hole in the items array, so we use the last item in the array to fill the hole, and update the corresponding index(which belongs to the last item) value in the items hash table.

@ax-jason ax-jason changed the title Add using_orderedQuery option to newWorld Try using array table for cell items Nov 11, 2016
@kikito
Copy link
Owner

kikito commented Nov 11, 2016

When removing a item, there will be a hole in the items array, so we use the last item in the array to fill the hole, and update the corresponding index(which belongs to the last item) value in the items hash table.

Thank you for explaining this. Here are some more comments:

  • You can probably rename getArrayItemsInCellRect to just getItemsInCellRect.
  • Please rename the vars items_dict and items_array to camelCase (itemsDict, itemsArray)
  • Instead of using table.insert in to create the itemsArray, define another variable called len and increase it every time you add a new element to it. This is a bit faster.
  • Similarly, if you return itemsArray, len instead of just itemsArray you can use len to iterate numerically, instead of using ipairs. This is also a bit faster.

use len to iterate array numerically instead of using ipairs.
@coveralls
Copy link

coveralls commented Nov 11, 2016

Coverage Status

Coverage increased (+0.05%) to 97.61% when pulling 27d1c96 on ax-jason:master into 4d776be on kikito:master.

@ax-jason
Copy link
Author

You got it.

@kikito
Copy link
Owner

kikito commented Nov 11, 2016

Thank you. Now the code looks quite nice. I must do some performance tests now. My intuition tells me that this solution will employ more memory (that much is obvious, since where we were using one dictionary, now we have a dictionary + an array), but I am not so sure about how will speed change. Adding and removing objects should be slower, but parsing neighbours should be faster. I can't know for sure. So I must do some performance tests comparing the old with the new (It might take me a while).

@ax-jason
Copy link
Author

ax-jason commented Nov 12, 2016

Patience makes perfect work. I admire you for this.
Yes, it will cost more memory and slower, which was why I made the switch option at first.
The whole thing began with a requirement of my project, which I need to ensure the same results for both server and client side simulation. So I found your beatiful lib, and I couldn't help to contribute the solution.
It's ok if the perfomence test didn't pass, the lib is for every one after all, just want to thank you again for the lib, it really helps and teaches me a lot.

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