Skip to content

Folia Scheduler : Do not throw an error if the delay is set to 0, just like for entities.#11339

Closed
Euphillya wants to merge 1 commit into
PaperMC:masterfrom
Euphillya:folia-delay
Closed

Folia Scheduler : Do not throw an error if the delay is set to 0, just like for entities.#11339
Euphillya wants to merge 1 commit into
PaperMC:masterfrom
Euphillya:folia-delay

Conversation

@Euphillya
Copy link
Copy Markdown
Contributor

On the entity scheduler, we see that a check using the Math.max method exists, but not on the others. If you set it to 0, you just get an error, so I modified it to default to the next tick instead.

@Euphillya Euphillya requested a review from a team as a code owner August 29, 2024 14:36
@Euphillya Euphillya changed the title Do not throw an error if the delay is set to 0, just like for entities. Do not throw an error if the delay is set to 0, just like for entities. Folia Scheduler Aug 29, 2024
@Euphillya Euphillya changed the title Do not throw an error if the delay is set to 0, just like for entities. Folia Scheduler Folia Scheduler : Do not throw an error if the delay is set to 0, just like for entities. Aug 29, 2024
Comment thread patches/server/0848-Folia-scheduler-and-owned-region-API.patch
@Machine-Maker
Copy link
Copy Markdown
Member

If setting the delay to 0 leads to an exception somewhere (idk if this happens for sure, just assuming), then either that exception should be fixed, or if that isn't feasible, 0 just shouldn't be allowed as an argument value.

@Warriorrrr
Copy link
Copy Markdown
Member

The exception is from inside the method itself, clearly stating that the delay/period must be greater than 0, which makes sense to me. The only way that 0 as an input is feasible is to Math.max it like done here

@Euphillya
Copy link
Copy Markdown
Contributor Author

Euphillya commented Aug 30, 2024

If setting the delay to 0 leads to an exception somewhere (idk if this happens for sure, just assuming), then either that exception should be fixed, or if that isn't feasible, 0 just shouldn't be allowed as an argument value.

It is not allowed precisely. But on the entity scheduler, we can set it to 0 but it is corrected by a Math.max so that the lowest value is 1 (see here:

+ this.oneTimeDelayed.computeIfAbsent(this.tickCount + Math.max(1L, delay), (final long keyInMap) -> {
)

@Machine-Maker
Copy link
Copy Markdown
Member

After further consideration, we thinking throwing is the more correct behavior. It's odd to clamp values to 1 instead of specifying what values are valid in the first place and disallowing other values. A PR adding a thrown exception for the EntityScheduler#execute method which I think is the only method that doesn't throw out of all the scheduler methods.

@Euphillya Euphillya deleted the folia-delay branch September 27, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

4 participants