How about a method of CollisionResults that returns just the geometries of all the collisions? Now multiple CollisionResult can contain the same geometry with a difference in collision mesh, but if the user is interested just in the geometries. I think it makes sense.
Sure It’s possible for the user to implement it by himself, but now I’m thinking of not so clean Hashmap implementation, or maybe some functional programming. But I’m not experienced in functional programming in java.
I
Cannot resist proposing xtend based solution 
results.map[geometry].toSetJava 8 would be a lot worse
StreamSupport.stream(results.spliterator(), false).map(coll -> coll.getGeometry()).distinct().toArray();
1 Like
Thanks for the answer 
I checked xtend, it looks kinda like Scala. but with many typing cheats 
For now I prefer to stick with Java 8 (also thinking to use guava lib in my project)
This might be a bit off topic question, but can I use Java 8 in jme3 if I want my game to run on Android?
I tried to google Android and Java 8 compatibility but didn’t find an unanimous answer
I don’t think that you can use java 8 on Android at the moment. But you CAN use xtend on Android Problem is that xtend doesn’t work with Netbeans, so it is not really a reasonable option if you want to use jme3 IDE.
Another suggestion: instead of CollisionResults implementing iterable, just exposing the internal ArrayList of results directly (or wrapped with Collections.unmodifiableList) seems like it would be preferable It’d make for a simpler CollisionResults object (wouldn’t have to re-implement the Iterable API and bits of the Collection API, like size) and would make it a little easier for clients, too. For example, assuming “results” is of type List<CollisionResult>, your Java 8 code would actually look like this:
[java]
List<Geometry> geometries = results.stream().map(CollisionResult::getGeometry).distinct().collect(toList());
// or, if you don’t care about ordering (analogous to xtend example above)
Set<Geometry> geometries = results.stream().map(CollisionResult::getGeometry).collect(toSet());
[/java]
Not as concise as xtend obviously, but not terrible.
I think I see why CollisionResults was implemented as it is – so setters could modify the internal structure in constant time and getters could lazily sort the internal structure. Still…a priority queue would probably have been a simpler choice, even if it meant O(log n) insertions. If anyone has any more background than this on why it was implemented this way, I’d be interested in hearing it.
Given the level of code it would break, I doubt we’ll be getting rid of any methods. I mean, I hope they are rarely used but I’m sure they are by lots of bad code everywhere. 99% of cases probably only grab the nearest result, after all. I think the need to hit each geometry only once is a fairly rare use-case, for example… and not too badly accomplished just in direct code (one extra line per loop: if( visited.add(geom) ) { do stuff })
And it’s funny, because potentially using a better sorted data structure internally is exactly why I think it would be bad to return List directly. As it stands, I think PriorityQueue is probably a net loss. Insertions would cost more and the list might be small or already sorted. I haven’t done the math, though. It also makes it impossible to iterate in order or by index without draining the queue first… which means an additional data structure and copy.
Oh no, I’d never mean to remove methods or somehow make backwards incompatible – I was just hoping to find a solution that would make everyone happy. Along those lines, I think I misspoke – while I believe an Iterable is too general, a List is probably indeed too specific, since PriorityQueue doesn’t fall under there. So why not a Collection?
Speaking of performance, there are a few different angles. Asymptotically, Collections.sort() require n lg(n) time if the input is unsorted (n if it is already sorted, something in between if it’s partially sorted). PrioritityQueue takes lg(n) time per insertion, which amounts to n lg(n) time for n insertions. If we can assume the CollisionResult objects are added to the CollisionResults object in seemingly random order, then the asymptotic runtime performance is about the same. There is one way in which Collections.sort is definitely more expensive – it dumps the entire backing array to a new array, and then when sorting that new array it creates yet another copy.
A major problem for PriorityQueue that I just came across is its iterator() method doesn’t iterate in prioritized order (which you pointed out in your post, but I didn’t notice/understand)! So that’s out. The only other real alternative in the standard library here is TreeSet, which doesn’t allow random access (which the CollisionResults API already exposes) and has the additional constraint of requiring unique elements (which isn’t bad but not free to enforce). So the approach of lazily sorting a list stays remains the only option.
Returning the backing ArrayList like so would still be feasible:
[java]
public Collection<CollisionResult> getResults() {
if (results == null) {
return Collections.emptyList();
}
if (!sorted) {
Collections.sort(results);
sorted = true;
}
return results;
}
[/java]
though I’d be inclined to actually return an unmodifiable array, lest someone somewhere gets the results and starts adding to it, thinking it’ll stay in order. That “someone” could be a caller within the jME platform itself, too.
Sorry for the ramble. Thoughts?
I suppose it’s no big deal but I don’t really understand what returning collection over just using iterable buys you. Any decent ‘functionalo’ API would support Iterable just as well as Collection. I know in Guava I can write a ‘distinct’ predicate that would work with either.
http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/collect/Iterables.html#filter(java.lang.Iterable,%20com.google.common.base.Predicate)
If Java8 doesn’t let you filter an iterable then that’s a shame.
After all, the only thing the Collection interface provides over Iterable in this case are size() and contains()… the latter of which doesn’t even seem useful in this case.
I guess the confusing thing to me is that CollisionResults looks, sounds, and acts like a collection, but isn’t, probably because implementing the collection interface would be a bit silly (and a moderate amount of work).
If all clients will ever be doing is iterating over the results, then yes, Iterable and Collection can be used interchangeably – but, if one wants to get particular elements, or the size, then it makes sense to expose the entire Collection instead of Iterable-plus-a-few-methods. This post denouncing Iterable as a return type http://alexruiz.developerblogs.com/?p=2519, while not something I completely agree with, brings up some interesting counterpoints in the comments – Iterables are good for one thing: lazy loading. That is, if collision results weren’t calculated until they were iterated over, then Iterable would make more sense. But I doubt that will ever be the case.
The new and improved way to iterate over things in Java 8 is using Streams. Iterables can’t be trivially converted to Streams; Collections can.
To wrap this up, there are really only two issues here, as far as I’m concerned:
- The interfaces/return types should be the most specific the platform developer can manage, while still preserving enough flexibility to change implementations in the future. I believe this would be a Collection (or possibly even a List) rather than an Iterable.
- CollisionResults contains something like half a SortedList implementation – why not pull that out into a full SortedList implementation and use and return that? (SortedList wouldn’t be that big, either – I implemented one as an exercise and it came out to about 90 lines).
@nanodeath said: if one wants to get particular elements, or the size, then it makes sense to expose the entire Collection instead of Iterable-plus-a-few-methods.
… Collection provides no way at getting at particular elements. To implement Collection, CollisionResults would have to add a lot of methods that only throw new UnsupportedOperationException()… not to mention methods like contains() would be completely useless.
All returning the internal list would do is give you ‘generic’ access to things you can already do on CollisionResults. And it seems the only point of that would be to support other collection-using APIs… but as mentioned, the only real use-case I can see for such a thing can be done with Iterables. So while adding a getList() method is not much code it also basically provides no value at the expense of “yet another few lines of code to maintain”.
The link you provided is interesting but amounts to “Iterable is always bad… update: oh, wait, except in all of these cases where it’s not”. Iterable is good for foreach… but it’s also good for the case where you want an Iterator that you can reset. (Something I sorely missed in Java for a long time.) It’s dumb that it doesn’t implement isEmpty(), though.
To the OP, in regular non JDK8 Java using Guava (and really everyone should be using Guava):
[java]
public class DistinctGeometry implements Predicate<CollisionResult> {
private Set<Geometry> visited = new HashSet<Geometry>();
public boolean apply( CollisionResult cr ) {
return visited.add(cr.getGeometry());
}
}
…some code somewhere…
for( CollisionResult cr : Iterables.filter(myResults, new DistinctGeometry()) ) {
…do some stuff…
}
[/java]
Thanks for the Guava code, for now I just implemented the DistictGeometry class as a straight forward filter function returning geometries Set.
I suppose this Guava way is more ‘functionally clean’, and easier to combine in conjunction with other methods.