Skip to content

Ordering + raw methods instead of reversing.#497

Merged
empirephoenix merged 5 commits into
jMonkeyEngine:masterfrom
NemesisMate:patch-7
May 22, 2016
Merged

Ordering + raw methods instead of reversing.#497
empirephoenix merged 5 commits into
jMonkeyEngine:masterfrom
NemesisMate:patch-7

Conversation

@NemesisMate
Copy link
Copy Markdown
Contributor

Continuing the discussion in the pull request: #471, made the requested changes.

In some cases, native-bullet returns the ray results on a reversed order so it leads to unexpected bugs.
@pspeed42
Copy link
Copy Markdown
Contributor

Hmmm... I wonder why the code used LinkedList... it will be less optimal in many ways but especially for sorting.

@NemesisMate
Copy link
Copy Markdown
Contributor Author

Yes, I had the same wonder but without more knowledge I am reluctant to change it (that's why I asked here at first place)

@pspeed42
Copy link
Copy Markdown
Contributor

LinkedList is only more optimal in the cases where one is adding or
removing from the front or back of the list. Else ArrayList is more
optimal in all cases... but especially for Collections.sort() which must
index into the list. LinkedLists are horrible for indexing as it must
always iterate from the beginning. Also, as for garbage, an ArrayList will
have much less garbage since LinkedList will create an internal linking
entry per item.

On Thu, May 19, 2016 at 6:52 AM, NemesisMate notifications@github.com
wrote:

Yes, I had the same wonder but without more knowledge I am reluctant to
change it (that's why I asked here
https://hub.jmonkeyengine.org/t/native-bullet-raytest-randomly-giving-inverted-list/35511/2
at first place)


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#497 (comment)

@NemesisMate
Copy link
Copy Markdown
Contributor Author

I know all that theory, what I don't know is why it uses a LinkedList in first place. If the only reason is because of it was was just adding/removing, then I suppose it must be changed now (hm... or changed only for the sorting case?). Maybe changing it depending on the case would be the better but at cost of obtaining different list types on the different methods (with/without sorting).
So, the different solutions would be:

  1. Change LinkedList by ArrayList in both cases (the rayTest and the rayTestRaw).
  2. Change to ArrayList just for the rayTest case (sorted).

If you are more eager to one of them just say it and I'll adapt it.

@pspeed42
Copy link
Copy Markdown
Contributor

Just use ArrayList in both cases. Whatever the original reasons for
LinkedList they are not valid. Either now or ever depends on historical
perspective but it's just as likely a misunderstanding about optimization
on the part of the original author.

On Thu, May 19, 2016 at 7:12 AM, NemesisMate notifications@github.com
wrote:

I know all that theory, what I don't know is why it uses a LinkedList in
first place. If the only reason is because of it was was just
adding/removing, then I suppose it must be changed now (hm... or changed
only for the sorting case?). Maybe changing it depending on the case would
be the better but at cost of obtaining different list types on the
different methods (with/without sorting).
So, the different solutions would be:

  1. Change LinkedList by ArrayList in both cases (the rayTest and the
    rayTestRaw).
  2. Change to ArrayList just for the rayTest case (sorted).

If you are more eager to one of them just say it and I'll adapt it.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#497 (comment)

@empirephoenix empirephoenix merged commit 65fd742 into jMonkeyEngine:master May 22, 2016
@empirephoenix
Copy link
Copy Markdown
Contributor

Thanks for the work :)

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