search

hollow-cube / minestom-ce

1.20.4 Lightweight Minecraft server
https://minestom.net
Apache License 2.0
108 stars 36 forks source link

New event node priority system #108

Closed lordtgm closed 7 months ago

lordtgm commented 8 months ago

added new functions to EventNode setChildPriority declares absolute or relative priorities for children of the event node. the priorities are resolved into integer priorities in the "old" system.

TheMode commented 8 months ago

May be better to move the enums as subclasses in EventNode. Like EventNode.AbsolutePriority and EventNode.RelativePriority

lordtgm commented 8 months ago

May be better to move the enums as subclasses in EventNode. Like EventNode.AbsolutePriority and EventNode.RelativePriority

done refactoring. should I open a new request?

mworzala commented 8 months ago

I do not think that this should go into the main repo. It just sets priorities anyway so could be done as an external library and imported. I'm not a fan of adding a new graph solving library just for this.

Additionally, I do not believe this solves a problem for many people. For example, a library cannot use this because it does not have knowledge of event nodes beyond the ones it creates, so must default to absolute priorities which do not make much sense. For example, consider library A with events 1, 2, 3 with 1 before 2 before 3. Using this change it may assign for example priorities 1, 2, 3 respectively. Then library B might need to run a node between 1 and 2 for a completely unrelated reason (cancellation ordering, for example). In this case it is impossible for library B to correctly order that node without a hard dependency on library A which makes no sense.