Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add entity tag support #119

Merged
merged 11 commits into from
Oct 1, 2023
Merged

add entity tag support #119

merged 11 commits into from
Oct 1, 2023

Conversation

Quillraven
Copy link
Owner

@Quillraven Quillraven commented Sep 26, 2023

PR for #118 (WIP)

@LobbyDivinus : this is a very quick draft from my side. Basically, I just added two additional entity extension functions which should be sufficient to support tags. You can see the usage in the Test class.

The downside of this approach is that tag ids are part of component ids, meaning that they will be part of the normal component mask and "waste" space in the BitArray.

I haven't had a look at your fork yet. Will do that tomorrow!


TODO:

  • documentation
  • separate contains, has, hasNo extensions for tags
  • verify in example projects if this breaks something (everything must compile without doing any changes)
  • benchmarks: had no impact for me. I was a little concerned about the introduction of the UniqueId interface but seems to have no extra overhead or impact
  • (not part of this PR because this is not so easy to fix) check if EntityComponentContext can be refactored a little bit to avoid componentService.world.entityService calls

@Quillraven Quillraven added the enhancement New feature or request label Sep 26, 2023
@LobbyDivinus
Copy link
Contributor

Looks great for a quick draft!

One thing I'm concerned about is type safety. There's no difference between regular components and tags which could prevent you from doing bad things.

For example you could accidentally add a Tag object to an entity, then unset that tag and by doing so cause an invalid state where the ComponentsHolder has the Tag object but the entity's mask does not signal having it.

@Quillraven Quillraven added this to the 2.5 milestone Sep 27, 2023
@Quillraven
Copy link
Owner Author

@LobbyDivinus: I updated the draft now. Documentation is still missing. It is now possible to also use enums as tags (thank you for your fork, that gave me the idea on how to solve it ;) ). And I also included typealias for tags to make the code easier to read and understand imo.

About type-safety: I had a similar concern and tried a few things but I think we are lucky. You get compile errors when you do something wrong because a tag does not extend Component. It does not even interact with a ComponentsHolder. It is just a simple bit that is set in the compMask BitArray.

Here is the current test that shows the syntax and compile errors:

data object Visible : EntityTag()

class TestTagSystem(var ticks: Int = 0) : IteratingSystem(family { all(Visible) }) {
    override fun onTickEntity(entity: Entity) {
        ++ticks
    }
}

enum class TestTags : EntityTags by entityTagsOf() {
    PLAYER, COLLISION
}

class EntityTagTest {

    @Test
    fun testTagAssignment() {
        val world = configureWorld {}
        val entity = world.entity { it += Visible }

        with(world) {
            assertTrue(entity[Visible])
//             entity.getOrNull(Visible) // <- compile error because Visible does not extend Component
//             val cmp : Visible = entity[Visible] // <- compile error because Visible does not extend Component and therefore 'cmp' must be of type Boolean
//             assertTrue(Visible in entity) // <- compile error because Visible does not extend Component
//             assertTrue(entity has Visible) // <- compile error because Visible does not extend Component
//             assertTrue(entity hasNo  Visible) // <- compile error because Visible does not extend Component

            entity.configure {
                it -= Visible
            }

            assertFalse(entity[Visible])
        }
    }

    @Test
    fun testTagSystem() {
        lateinit var testSystem: TestTagSystem
        val world = configureWorld {
            systems {
                add(TestTagSystem().also { testSystem = it })
            }
        }
        val entity = world.entity { it += Visible }

        world.update(1f)
        assertEquals(1, testSystem.ticks)

        testSystem.ticks = 0
        with(world) { entity.configure { it -= Visible } }
        world.update(1f)
        assertEquals(0, testSystem.ticks)
    }

    @Test
    fun testEnumTags() {
        assertNotEquals(TestTags.PLAYER.id, TestTags.COLLISION.id)
        assertNotEquals(Visible.id, TestTags.PLAYER.id)
        assertNotEquals(Visible.id, TestTags.COLLISION.id)

        val world = configureWorld {}
        val entity = world.entity { it += TestTags.PLAYER }

        with(world) {
            assertTrue(entity[TestTags.PLAYER])

            entity.configure { it -= TestTags.PLAYER }

            assertFalse(entity[TestTags.PLAYER])
        }
    }
}

@LobbyDivinus
Copy link
Contributor

Ah, with that forget about my concerns. This looks quite polished and covers all of what I'd expect tags to do.

@Quillraven
Copy link
Owner Author

Quillraven commented Sep 27, 2023

Wow, that took quite some time to verify it in my three example games because a lot of things got actually broken haha. Luckily I drank an energy drink today so going the extra mile for my community and work until midnight was not an issue!

To summarize it:

  • entity removal was broken when an entity had a tag (IndexOutOfBoundsException in bag). I fixed it by adding a new clearAndForEachBitset function to the BitArray
  • some other "null issue" with contains, has and hasNo functions
  • enum class tags could not be used to create families
  • snapshots did not support tags at all. To solve it, I now added a new Snapshot class that contains the components and tags of an entity. For tags this was a little bit tricky because there is no ComponentHolder for a tag and I did not want to introduce one because then we could really just stick to components. So I solved it by adding a tagCache to the world and every time a tag gets added to an entity then it is also added to the cache. That way I can return the correct tag instance for a snapshot. I think this should work because if you never assign a tag to an entity then its bit in the component mask should never be set and therefore it should never be part of a snapshot and so it is okay if it is missing from the cache.

@LobbyDivinus: Would be great if you can check my changes and review them and ideally also try this branch in your local game and experiment a little bit with it.

@Quillraven Quillraven marked this pull request as ready for review September 27, 2023 21:36
* add plusAssign for list of tags
@LobbyDivinus
Copy link
Contributor

LobbyDivinus commented Sep 28, 2023

Thank you, that was quick and it looks great so far! I will report back once I've had a chance to play with it more thoroughly. I don't use snapshotting though so I wouldn't be able to notice issues there.

@Quillraven Quillraven changed the title add entity tag support (WIP) add entity tag support Sep 30, 2023
@Quillraven Quillraven merged commit c677f2e into master Oct 1, 2023
4 checks passed
@Quillraven Quillraven deleted the #118_entity-tags branch October 1, 2023 06:36
@LobbyDivinus
Copy link
Contributor

Sorry for the late reply, I did play a bit more with it and integrated it into may project and it all works as expected. There seems to be no noticable difference in performance but definitely in memory use. Thanks again for adding this one.

@Quillraven
Copy link
Owner Author

No worries, I just wanted to merge it today to have it officially in the snapshot version.

I want to release 2.5 when Kotlin 1.9.20 is out for a week or two. Then this feature will be officially in Fleks stable versions 🙂

Thanks again for the suggestion and help with the enum class tags!

@LobbyDivinus
Copy link
Contributor

Sounds great. I really appreciate this extension and the fast way you respond to feature requests.

@Quillraven Quillraven mentioned this pull request Oct 17, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants