search

ondras / rot.js

ROguelike Toolkit in JavaScript. Cool dungeon-related stuff, interactive manual, documentation, tests!
https://ondras.github.io/rot.js/hp/
BSD 3-Clause "New" or "Revised" License
2.33k stars 254 forks source link

Scheduler.Action fails if you add a `0` as an actor. #164

Closed Hectate closed 5 years ago

Hectate commented 5 years ago

I was having some issues with implementing the scheduler and I was able to narrow it down to this issue. Because I'm using an ECS library, I was using the Entity IDs from that library to add them to my scheduler. The first entity from the library was id 0, and accordingly it was passed along to the Scheduler as the first actor to schedule. With a 0 as the first actor, the scheduler broke down and calling scheduler.next() never returned that first actor when it should have.

I was able to work around it by creating an Entity to not be used first so that all of the entities had non-zero IDs, and the scheduler began working as expected. While expecting a non-zero ID is not unreasonable, there was no documentation or notes that indicated that certain values were required/expected. Perhaps a note for the documentation?

ondras commented 5 years ago

Hi @Hectate ,

thanks for the bugreport! This is a nice one.

Perhaps a note for the documentation?

Nah, I would say that a numerical ID (incl. zero) should be fine. I would guess that we have a if (!actor) test or similar present; I will simply try to make this comparison more strict.

ondras commented 5 years ago

Fixed! Please verify by updating to version 2.1.1.

Hectate commented 5 years ago

Fix confirmed. Thanks!

Hectate commented 5 years ago

I didn't use it, but I noticed this was still using the old style check: https://github.com/ondras/rot.js/blob/9fa25bdbe50a1c85c718542aeec4365345c7e445/src/scheduler/speed.ts#L26

I had issues with a ID 0 again today, but it's really strange and inconsistent. I'm going to use my workaround for now and later when I have time I'll build a barebones test case to narrow down the issue. Thanks again.

ondras commented 5 years ago

I didn't use it, but I noticed this was still using the old style check:

Speed actors are supposed to provide a getSpeed() method, so 0 is not supposed to work here.

Hectate commented 5 years ago

You're correct! My bad.