Possible bug in Family.Builder

[Lusito]

I was just looking through the changelog and noticed this commit: https://github.com/libgdx/ashley/commit/a9b3eea07e9dbc81860bc44b4e826773209d204a

The call for reset() was moved from before creating a Family to after creating the Family. Imagine a setup like this:

Family family = Family.all(getAll()).one(getOne()).get();

Now if getOne() throws an exception, Builder.all will not be reset, creating a bad family on the next call to Family.all|one|exclude.

In a case like #137, which instantiates its own Family.Builder Object (which should not be needed in most cases), there still is no need for this: If you want to reuse the custom builder instance after get(), just call reset() on it manually.

Having reset() called inside of get() also causes another problem in this situation:

Builder builder = Family.all(x);
Family a = builder.get();
Family b = builder.one(y).get(); // you'd expect b == Family.all(x).one(y).get(), but you'll get b == Family.one(y).get();

[54k]

I think the simplest approach is just creating new builder in all static methods, like artemis odb does. Or document "reset" behavior. I think the first solution is more predictable.

[Lusito]

I'd say it's up to the user. If the user wants a unique instance, he can just create a new builder. In most cases you can reuse the single instance.

What exactly do you mean by document reset behavior ?

[54k]

I mean to add a note about reset feature into the builder's get method javadoc. Sorry for engrish.

Upd: anyway I think that the shared builder is unnecessary. And leads to bugs that you described.

[Lusito]

ah, documentation, I thought about a document ^^, yes sure.

I think wasting a builder every time you want a family is unnecessary: new Builder().all(...).one(..).get() seems wasteful to me.