Use default getindex for vector[face] when no Polytope is generated

[ffreyer]

This allows things like f[f .== 1]

[SimonDanisch]

That seems pretty breaking to me!

[ffreyer]

The only thing that changes is that a tuple becomes a Vector

[SimonDanisch]

But that code path is usually used in high performance situations (e.g. iterating over all face indices)...
I think this could have quite the impact to use vectors there...
Maybe we can return a FixedVector{Bool} instead? I guess that should have the same indexing behaviour as vector.

[SimonDanisch]

Ok, I think this is the right approach to fix #249 and #247
We also need to remove:

@propagate_inbounds function Base.getindex(points::AbstractVector{P}, face::F) where {P<: Point, F <: AbstractFace}
    return Polytope(P, F)(map(i-> points[i], face.data))
end

@propagate_inbounds function Base.getindex(elements::AbstractVector, face::F) where {F <: AbstractFace}
    return map(i-> elements[i], face.data)
end

Looks like that was the behaviour before, and I dont see a good reason why whe should deviate from the previous behavior.

[SimonDanisch]

I guess the tests should be:

f = QuadFace{Int64}(1,2,3,4) # Some face
@test f[f.!=3]  isa AbstractVector{Int} # [email protected] had Vector, but maybe SVector would be better here
V = Point{3, Float64}[[-1.0, -1.0, -1.0], [-1.0, 1.0, -1.0], [1.0, 1.0, -1.0], [1.0, -1.0, -1.0]]
@test V[f] isa typeof(V) # 0.4 returned Vector

[Kevin-Mattheus-Moerman]

@ffreyer yep this seems to fix those issues, thanks!

• Using faces to index into point vectors no longer produces point vectors #247 is fully fixed.
• The core issue in Breaking behaviour change relating to using Boolean operations on faces #249 i.e. allowing for f[f.!=3] on faces, is fixed.
  A side note is that if we have f = QuadFace{Int64}(1,2,3,4) then f.!=3 still returns the odd creature that is a QuadFace{Bool}. Similarly for a Point{3,Float} by the name of v this v.<0.5 returns a Point3{Bool}. Both these face/point types seem to never actually represent a valid face or point in any way. For my projects this is not breaking anything but it does seem like perhaps these things should return some kind of static vector containing Bools? Again this doesn't break anything, and perhaps you have good reasons for keeping it.

[SimonDanisch]

We decided to keep it like this for now to just recover the old behaviour as quickly as possible..
I agree, that this should in the future rather return an SVector, but we didn't like to get into a bigger refactor, this may entail.
Although since QuadFace is an SVector in disguise, it probably would be relatively quick to change.