Text should be rendered via drawable component

[fallahn]

Currently text components contain their own vertices instead of implementing the drawable component, and therefore get rendered with their own text render system. The only current advantage of this is the ability to crop texts - although this property could be moved to the drawable component. Ideally text would be drawn as other drawables, allowing it to be depth sorted with sprites and custom drawable implementations.

[fallahn]

@JonnyPtn any chance you could test the text_system branch / offer feedback? I may have broken the immediate text bounds update.. :S Thanks!

[JonnyPtn]

Demo looks fine to me, will test more thoroughly and check the bounds on creation in the next few days

[JonnyPtn]

I've tested in some other projects and the text rendering itself seems to be working fine, although getting local bounds on creation is indeed broken.

It's a tough one to solve, as I agree the system should be responsible for updating the vertices, but then it becomes tough to solve instances like this:

e.addComponent<xy::Drawable>();
e.addComponent<xy::Transform>();
e.addComponent<xy::Text>(font).setString("New game");
e.getComponent<xy::Text>().setCharacterSize(50);
e.getComponent<xy::Text>().setFillColour(sf::Color::White);
auto bounds = e.getComponent<xy::Drawable>().getLocalBounds();
e.addComponent<xy::UIHitBox>().area = bounds;

Without having some interaction between the text/drawable components.

The only thing which springs to mind (and I'm aware this isn't perfect...) would be to make the render system responsible for the bounds, something like xy::RenderSystem::getLocalbounds(xy::Entity) which checks for all drawable components and makes sure their bounds are up to date.

I'll keep having a think and let you know if anything springs to mind...

edit: also slightly off-topic but you can make the deprecation warning compile time using these fancy c++14 decorators, e.g.:

[[deprecated ("Use Drawable::getLocalBounds() instead.")]]
sf::FloatRect getLocalBounds() const;

[fallahn]

Thanks for taking a look. That's not a bad idea, querying the render system. I'm almost tempted to make it a static function... I just need to make sure the render system's bounds calculation returns the same results as the text system as they appear to work slightly differently. Also thanks for the decorator tip - I'll give those a try!

[fallahn]

Hm, the more I think about this the more complicated it gets. It's one thing to query the drawable's local bounds - but it needs to trigger a vertex update (assuming the vertices are flagged dirty) on an arbitrary component. It's not just text but also sprites and any other custom drawable whose vertex update is delegated to another system which somehow needs to be hunted down and triggered before querying the bounds. Further head scratching required...

[JonnyPtn]

Also worth mentioning - any querying through the system would need some refactoring of how entities are added to the system. Currently they are added during scene.update() so the system won't be aware of the entity/component until the next frame after creation anyway...

[JonnyPtn]

Another idea which crossed my mind (for better or worse) would just be to get rid of the drawable component and render system (or maybe just make them abstract classes) then have the sprite system draw the sprites, text system draw the texts, etc...

[fallahn]

This is how I had it set up at first, but it made it difficult to create custom drawables which would then also need their own rendering system, as well as making it impossible to sort draw depth between different types. I think this PR is on the right track, it's only a matter of solving immediate updates of drawables as they are created

[JonnyPtn]

Maybe just move the bounds functionality to the system (I'd say that's in line with the pure ECS approach) which can access the dirty flag of the component and will know whether the verts need to be updated beforehand or not.

If that's changed, then that just leaves the problem that entities aren't registered with the systems until the frame after the component is added, which can be addressed as a separate problem (I have a few suggestions on that front too which I've been playing around with)

[fallahn]

As it is at the moment it's down to the text or sprite system to update the bounds of the drawable as needed - which is fine because at that point the system has access to all the components it needs.

As you say, it would be useful for systems to be updated with new entities immediately (or at least sooner than they currently are). If onEntityAdded() was called as soon as the text component was added then it would be trivial here to update the vertex array based on its current status, although this has 2 problems: the text might not be in a ready state (ie the string not set, char size not set etc) and the entity might not yet even have a drawable component added... a system needs to be sure that all the components in its requirement bitmask are present before onEntityAdded() is performed. At least with the delayed update you have a better gaurentee that the entity is in a state where it is ready to be updated.

[JonnyPtn]

Might be interested in something I was playing around with recently here

Means entities have their components declared on creation, and they can subsequently be added to the systems immediately on creation. I quite like it, and the way it makes the make-up of an entity quite clear on creation, for example:

auto buttonEntity = m_scene.createEntity<Drawable, Transform, Text, Sprite, UIHitBox>();
buttonEntity.getComponent<Text>().setString("boop");
buttonEntity.getComponent<Sprite>().setTexture(tex);
buttonEntity.getComponent<UIHitBox>().area = m_scene.getSystem<SpriteSystem>().getLocalBounds(buttonEntity);

That's a loose proof of concept I tested, and it allows you to be sure that an entity has the correct components immediately on creation, and you can query systems immediately after an entity is created. You can then have the systems responsible for making sure components are updated if required (for example getting the bounds after a text has been modified) and it can only really screw up with a user error (for example getting the bounds before modifying the text). If you think it's viable it could probably be smartened up a little bit

[fallahn]

Ooh now this is a very interesting idea. The list of template parameters might get a bit long on more complex entities, but I can live with that - the only query I have is how would you pass constructor parameters to any components that might need them?

[JonnyPtn]

Yeah that's where It started getting complicated so I just got distracted :P

I remember I got something vaguely working, but the syntax just got pretty messy, especially when you take into account that some components won't have constructor parameters.

One of the attempts I made was just to pass the pre-built components into the function so it could auto-deduce, e.g:

xy::Transform t;
xy::Drawable d;
xy::Sprite s(texture);
m_scene.createEntity(t, d, s);

but there was some other issue with that which I don't remember... I think I had trouble getting it to work with transform which doesn't have a copy constructor or something. Could be workable though?

edit: also you wouldn't necessarily have to prevent people adding components after creation, it would just be a caveat that it would be a delayed update

[fallahn]

Yeah this sounds promising, I was about to suggest something along these lines before I read this. Transform components are indeed non-copyable though so require properly forwarding. Could be doable though!

[JonnyPtn]

After much poking, i'm not convinced it can be done in a nice way without making transform copyable. What happens to the local component the user has created before they add it to the entity?

Is there a reason for transform to be non-copyable? I don't think any other components are, and it also kind of goes against the pure ECS design where components should ideally be plain data (which is trivial to copy)

[fallahn]

Transforms are a special case unfortunately because of how the scene graph works. For example if you copied a transform with a series of children then those children would have two parents. Or the copy would have children who think they're still parented to the original or... and then my brain turns to mush.

[JonnyPtn]

I see... another issue to ponder...

[JonnyPtn]

There's only really 2 viable solutions I can think of:

• Make the addComponent overload move instead of copy the component it's passed (potentially using unique_ptrs)
• Implement Transform copy semantics in which the new copy is added as a new child of the original parent, but doesn't copy any of it's children. I know it's not perfect, but seems like the most logical thing I'd expect to happen when copying a transform.

[fallahn]

Moving is no problem, that's what the entity manager does anyway.

I've had another thought (albeit a shower thought so I might be missing something) how about something like:

static sf::FloatRect xy::Text::getLocalBounds(xy::Entity entity)
{
    XY_ASSERT(entity.hasComponent<xy::Text>() && entity.hasComponent<xy::Drawable>(), "Invalid Entity");

    auto& text = entity.getComponent<xy::Text>();
    auto& drawable = entity.getComponent<xy::Drawable>();
    if(text.m_dirty)
    {
        sf::FloatRect bounds;
        text.updateVertices(drawable.getVertices(), bounds);
        drawable.updateLocalBounds(bounds); //side note: not sure why I'm not just passing the drawable to the above function...
    }
    return drawable.getLocalBounds();
}

It goes back to your previous idea, but specialises for the Text component. Then it's up to custom drawables to implement this if it's deemed something that's needed. Sprites store their own texture rects already which can be used to query the local bounds so they don't really need anything else.

[JonnyPtn]

I don't see any problem with that, so I'm happy to go along with your preference. Am I right in thinking that doesn't require any changes to the entity creation either, given that it's the user responsibility to ensure both components have already been added? (Not saying we shouldn't consider changing entity creation in the future though)

[fallahn]

Yeah this leaves entity creation as-is so that can be dealt with in a separate issue. The assert should push the user in the right direction if there's an error I think. I'll make these changes now :)