New Entity iteration: more Julian, more elegant, more better

[louisponet]

Goal

Following the discussion in #6 the (so-far) ideal goal would be to allow changing

function Overseer.update(::LifetimeUpdate, m::AbstractLedger)
   timer = m[TimerComp]
   lifetime = m[LifetimeComp]
   for e in @entities_in(timer && lifetime)
       if timer[e].time > lifetime[e].max_age
           schedule_delete!(m, e)
       end
   end
   delete_scheduled!(m)
end

into

function Overseer.update(::LifetimeUpdate, m::AbstractLedger)
    for e in @entities_in(m, TimerComp && LifetimeComp)
        if e.time > e.max_age
            schedule_delete!(m, e)
        end
    end
    delete_scheduled!(m)
end

Thoughts

I tried previously something similar using NamedTuples but those copy data and so it didn't feel the most optimal solution.

Maybe some form of NamedTuple with type stable pointers may work. The Entity itself will also have to be stored in order to allow for the schedule_delete!(m, e).

[ffreyer]

For reference https://github.com/JuliaPlots/AbstractPlotting.jl/pull/507 might have some useful info about getting a type stable getproperty.

Since the ledger is passed to @entities_in here, the resulting e could also just be a ledger + entity. Then you could do e[TimerComp].time quite easily, right? I don't think getting e.time to work is actually possible since there's no guarantee that LifetimeComp doesn't have a field time as well...

[louisponet]

Right, that's a good point about the field names.

e[TimerComp] would be like e.ledger[TimeComp][e.entity] in the straightforward implementation of the suggested case. Definitely something to play around with.

I have been wondering if some kind of macro that works on the entire update function couldn't work, i.e. translating things like LifeTime[e] to first a local reference: loc_LifeTime = ledger[LifeTime] in the "preamble" of the function, followed by a replacement of LifeTime[e] to loc_LifeTime[e] in the loops. Maybe that would alleviate already some of the tediousness, but not quite the final result of e.time, which might be impossible for the reasons above.

In any case, I will have a look at how it works in the SOA package, but I think the crucial difference is that there they know that no 2 fields will have the same name.

[louisponet]

I've made an initial implementation using pointers in branch new_iteration.

This allows for functionality like:

function func1(m::Ledger)
    v1 = 0.0
    v2 = 0.0
    spat = m[Spatial]
    spring = m[Spring]
    @inbounds for e in @entities_in(spat && spring)
        v1 += e[Spatial].position[1]
        v2 += e[Spring].spring_constant
    end
    return v1 + v2
end

function func2(m::Ledger)
    v1 = 0.0
    v2 = 0.0
    @inbounds for e in @entities_in(m, Spatial && Spring)
        v1 += e[Spatial].position[1]
        v2 += e[Spring].spring_constant
    end
    return v1 + v2
end

Now, obv func1 is a bit silly, but it demonstrates that the speed of this implementation can be even better than the original (to be used on the master branch):

function func1(m::Ledger)
    v1 = 0.0
    v2 = 0.0
    spat = m[Spatial]
    spring = m[Spring]
    @inbounds for e in @entities_in(spat && spring)
        v1 += spat[e].position[1]
        v2 += spring[e].spring_constant
    end
    return v1 + v2
end

The reason why func2 is slow atm is that for now I did not implement a smart macro that locally defines sym1 = m[Spatial]; sym2 = m[Spring]; for e in @entities_in(sym1 && sym2), and m[Spatial], m[Spring] is done each evaluation of the test function. To be honest the macro is a little backwards written and I got a bit lost in it for now :sweat_smile:. Will work on that.

Anyway I think this is a nice POC, and I'd like some feedback. One clear thing that's less flexible is that now you don't have direct acces to the Entity anymore inside the loop, so creating new Components for a given entity is not possible. However, I think enumerate(@entities_in(xyz)) that returns something like (Entity, (ptr1, ptr2, ...)) is not a bad solution. I think the latter will be a little slower than the current master implementation, but as creating entirely new components for entities is usually only done sporadically (think uploading some openGL buffers from meshes the first iteration etc), I think it's not too bad.

Let me know what you think, especially about the use of pointers and unsafe_load.

Cheers!

[louisponet]

Okay speed is fixed. From what I can tell, func2 is now faster than func1 used to be on the prev master!

[louisponet]

e[Spatial] = Spatial(...) is now also supported through unsafe_store!

[louisponet]

Okay, I think most of the functionality is there, only things like schedule_delete etc that work with normal entities should probably also work for the new EntityState. Tests work, except for the iteration over the current rudimentary groups.

BTW, I had to put some additional checks etc in order to support || in the @entities_in definition, which leads to about an 8.4% speed decrease even when not using ||. I see 2 ways around: 1. stop supporting ||, I don't know how useful it is anyway. 2. Define a separate iterator for when only && is used (seems a bit messy).

[ffreyer]

For my second round of "try to convert MakieCore to an ECS/Overseer based approach" I've been trying to have both batched (i.e. Vector) component and single-value components. I ended up writing a monster along the lines of @entitites_in((BatchedA || InstancedA) && (BatchedB || InstancedB) && ...) there, which led to a lot of a = e in BatchedA ? m[BatchedA][e] : m[InstancedA][e] which seems like a bad pattern to me. I'm starting to wonder if || is just bad in general.

Though on the other hand I don't know how else to handle the 6 or so components that have this kinda structure. Should I generate 2^6 loops with all the possible mixtures? GLMakie lazily generates a shader for each possibility here atm, if I'm not mistaken. Or should I make my components unstable/a union to avoid all the switching. Or perhaps view length-one arrays as values? I'm a bit lost with MakieCore, but that might make a good toy to figure out what Overseer should be able to do as well...

[louisponet]

This is exacly also what I always ran in to when I tried using it and, while checking whether it's in 1 or the other is pretty trivial with the current Ptr{Comp}() being inside the EntityState pointers, it still feels clunky.

Can something like

for c in (UniformColor, MeshColor)
    for e in @entities_in(m, c && Spatial)
         ....
    end
end

pretty much always do the trick?

[ffreyer]

Probably, but it's pretty awkward if you have a lot of components.

for spatial in (BatchedCairoSpatial, InstancedCairoSpatial)
    for marker in (BatchedCircleMarker, InstancedCircleMarker, BatchedCharMarker, InstancedCharMarker)
        for color in (BatchedColor, InstancedColor)
            for scale in (BatchedScale2D, InstancedScale2D)
                for offset in (BatchedOffset2D, InstancedOffset2D)
                    for strokecolor in (BatchedStrokeColor, InstancedStrokeColor)
                        for strokewidth in (BatchedStrokewidth, InstancedStrokewidth)
                            for e in @entities_in(
                                    spatial && marker && color && scale && offset && 
                                    strokecolor && strokewidth && Camera && Transform
                                )
                                cairo_render_with_that()
                            end
                        end
                    end
                end
            end
        end
    end
end

Not that it's less awkward with || though...

group = @entitites_in(
    (m[BatchedCairoSpatialXYZ] || m[InstancedCairoSpatial]) && 
    (
        m[BatchedCircleMarker] || m[InstancedCircleMarker] || 
        m[BatchedCharMarker] || m[InstancedCharMarker]
    ) && 
    (m[BatchedColor] || m[InstancedColor] ) &&
    (m[BatchedScale2D] || m[InstancedScale2D]) &&
    (m[BatchedOffset2D] || m[InstancedOffset2D]) &&
    (m[BatchedStrokeColor] || m[InstancedStrokeColor]) &&
    (m[BatchedStrokeWidth] || m[InstancedStrokeWidth]) &&
    m[Camera] && m[Transform]
)

Maybe components like that should just unions...?

[ffreyer]

On the topic of elegant, I think it would be nice if one could create a new group from an old group. For example:

@component struct InScreenSpace end

group = @entities_in(...)
for e in @entities_in(group && InScreenSpace)
    # handle attributes as given in screen/pixel coordinates
end
for e in @entities_in(group && !InScreenSpace)
    # handle attributes as given in world coordinates
end

[louisponet]

While I understand what you're saying, in practice is it that common that for all those combo of components you have exactly the same internal function?? Like 1 or 2 or 3 nested loops I could see happen, but that many in practice at least I've never ran into.

It kind of shares the question whether it should be possible to pull all the Spatial components from a ledger with ledger[Spatial] if there exists Spatial{Float64} Spatial{Float32} etc. That being said, in that case it makes sense to just have one spatial and convert the type on creation. But I could see some usefulness in being able to pull all components that have the same abstract ComponentData type.

[louisponet]

On the topic of elegant, I think it would be nice if one could create a new group from an old group.

Yes, that functionality does kind of exist but using real OrderedGroups etc. While it does look elegant, is it really that useful? Usually I only implement something that would actually make my life easier during my coding.

[ffreyer]

While I understand what you're saying, in practice is it that common that for all those combo of components you have exactly the same internal function?? Like 1 or 2 or 3 nested loops I could see happen, but that many in practice at least I've never ran into.

I mean it's what I'm struggling with in MakieCore. Those are the components I have for scatter, though the Spatial component should probably just be a Batched version. They're all "one value that applies to every element" or "many values where each value applies to one element". I haven't found a good way to handle that yet, and this doesn't seem like good one either.

It kind of shares the question whether it should be possible to pull all the Spatial components from a ledger with ledger[Spatial] if there exists Spatial{Float64} Spatial{Float32} etc. That being said, in that case it makes sense to just have one spatial and convert the type on creation. But I could see some usefulness in being able to pull all components that have the same abstract ComponentData type.

I'm worried direct input conversion would make things harder to work with as a user in Makie. Though it probably just ends up being color = :red becoming e.color = RGBAf0(1,0,0,1) which isn't too bad.

[louisponet]

I'm worried direct input conversion would make things harder to work with as a user in Makie.

Yea, the direct input for sure is also something I'm struggling with in Glimpse with all components having fixed types and I would like to see fixed.

They're all "one value that applies to every element" or "many values where each value applies to one element". I haven't found a good way to handle that yet, and this doesn't seem like good one either.

I'm not sure I understand fully. I've had a bit of the same issues in figuring out the border between cpu and gpu in Glimpse, but nothing like this. Not clear what the solution is imo.

[ffreyer]

I'm not sure I understand fully.

Makie allows scatter(points, color = :red) and scatter(points, color = [c1, c2, c3, ... cN]). And at no point is :red converted to an array of colors.

[louisponet]

Probably there's a way around having that many loops, for OpenGL stuff I usually manage to do first a loop over all different colors then the different Materials etc. I don't know how it works with Cairo exactly. It might be useful to have some kind of intermediate step where all the different ways of defining a color get translated to a single shared data structure.

I'm really not sure if there's a way of solving this on the package level.

[c42f]

I don't think getting e.time to work is actually possible since there's no guarantee that LifetimeComp doesn't have a field time as well...

Yeah this was also one of the difficulties I mentioned in https://github.com/louisponet/Overseer.jl/issues/6#issuecomment-830409041

My thought was that we should have some way to disambiguate where necessary — if the field names are going to be unique most of the time it seems a pity to disallow this really attractive option.

[louisponet]

Okay, I've managed to implement the getproperty based on Tim's nice implementation. Seems that there's not much, if any, impact on performance. I really like this as well, the main problem is indeed what to do with ambiguous cases.

In the current implementation I've made a warning at compile time that warns about this and that by default the field of the first ComponentData that has it will be used. The problem is that this also leads to many warnings whenever a function with AbstractEntity is used since it uses getproperty for e.id in the case that e <: EntityState. I.e. even if you're not using any of the new syntax you still might get the warnings. I'm not quite sure how to implement it in a better way, checking for the ambiguity at runtime seems like a big nono.

Alternatively we can just not print the warning and make it clear in documentation that the default would be as it is. Dunno if that is not very bug prone...

[louisponet]

Okay I've slightly changed the implementation so that the warning is generated only when the fieldname that is present in multiple of the ComponentDatas is called! I think this is quite ideal. Let me know what you think!

If you like this design I think it's pretty much ready to merge, I'll add some changes to the README and some tests.

[c42f]

Okay, I've managed to implement the getproperty based on Tim's nice implementation. Seems that there's not much, if any, impact on performance.

Nice!

Okay I've slightly changed the implementation so that the warning is generated only when the fieldname that is present in multiple of the ComponentDatas is called! I think this is quite ideal.

This sounds better indeed. Though even better to just make it an exception instead? A warning doesn't seem super useful because so much @warn in the inner loop will make this unusable in real code. The only time it'd be useful would be in prototyping.

I thought we could just have an additional naming convention which allows "generated field names" like e.ComponentTypeName_fieldname for times when there's ambiguity. Or maybe even allow e.ComponentTypeName , so that e.ComponentTypeName.fieldname would work. (Thought I think this latter naming wouldn't work setproperty!.)

If you like this design I think it's pretty much ready to merge

Sounds pretty great to me. Feel free to ping me on a PR if you want some actual code review.

[louisponet]

Or maybe even allow e.ComponentTypeName , so that e.ComponentTypeName.fieldname would work. (Thought I think this latter naming wouldn't work setproperty!.)

Isn't that similar to how it is now with e[ComponentTypeName].xyz? BTW I forgot setproperty! will do that now :P.

Though even better to just make it an exception instead?

Yes I'll turn it into an exception indeed.

[c42f]

Isn't that similar to how it is now with e[ComponentTypeName].xyz?

Oh I see, yes I think that makes a lot of sense.