Ground items kotlin #372
Ground items kotlin #372
Conversation
|
It'd be nice to get transactions in with this before kotlin-experiments is merged back to master. |
You can use the existing mocks of the |
Not sure I agree with this. kotlin-experiments is already massive, we were supposed to have feature-frozen it |
|
I agree with @Major-, I love the work that @ryleykimmel has done here, but it would be better to put merge this once kotlin-experiments is merged. Kotlin-experiments branch is already going to be a huge merge. |
|
That's fine. I'm more concerned with this making it to master before the transactional item stuff and then requiring a rewrite. |
|
FWIW the item transaction code I wrote many years ago should be scrapped. I don't remember the plan I had in mind when creating this and feel it could be (re)designed in a better manner. |
garyttierney
force-pushed
the
kotlin-experiments
branch
from
5211b9b to
97896a3
Compare
|
Need to decide whether or not this should be merged or should actually be in core (i.e. written in Java) |
Major-
added
blocked
| /** | ||
| * A [ScheduledTask] that manages the globalization and expiration of [GroundItem]s. | ||
| */ | ||
| class GroundItemSynchronizationTask(private val groundItem: GroundItem) : ScheduledTask(DELAY, false) { |
There was a problem hiding this comment.
I think this could be better done as a single task that polls GroundItem's from a queue every tick.
There was a problem hiding this comment.
Mind explaining how that queue should work? I don't think to queue them makes much sense here as all this task is doing is keeping the ground items state in sync with the client at the appropriate time.
There was a problem hiding this comment.
So there's one GroundItemSyncTask at all times and new ones are just added to its queue, rather than creating a new task for each item. Downside of this is we have to implement what is essentially scheduling logic in the task itself
There was a problem hiding this comment.
Kind of defeats the purpose of the task scheduler, does it not?
There was a problem hiding this comment.
Partially, yes; that is the downside
|
I'm looking at revisiting this. Can we come to a consensus on whether this should be core functionality or left alone in Kotlin? Personally I think it fits better in the core as this kind of thing isn't really fit for a plugin as it is core functionality to how ground items work, but I'm not sure. Ping @Major- @garyttierney |
|
@ryleykimmel also agree it should be in core |
Region