-
Notifications
You must be signed in to change notification settings - Fork 20
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
Celestial Events API #115
base: develop
Are you sure you want to change the base?
Celestial Events API #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment on the actual logic, I only gave this a cursory look before heading off for the night.
Suggestion: A few javadoc comments on the API classes at least would help folk figure out what's going on with celestial events.
...main/java/net/modificationstation/stationapi/api/event/celestial/CelestialRegisterEvent.java
Outdated
Show resolved
Hide resolved
The recent commit should properly address the requested changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine aside from the fact it should be using a registry for tracking registered events. I'll help you with that when I get a moment.
About the registry: I was actually considering to use it but was not sure if it makes sense. Is it about preventing events from having identical names or something else entirely? |
A potentially more significant issue I just realized is that I am trying to merge into the master branch while I should be merging into a different branch due to this being a new feature. Should I change the target branch if this needs to be done on my end? |
The newline should be taken care of now |
Added CelestialEvent. Added the Register Event. Added Test Items. Current Features: - Adjustable Intervals - Day Offset to offset the Interval - Custom Day Length - Custom Chance for successful Activation - Mutually exclusive Events # Conflicts: # src/test/java/net/modificationstation/sltest/item/ItemListener.java
Added Mixin for CelestialTimeManager. Celestial Events now automatically start and stop if added to the Time Manager. Starting Point can be customized to 4 Times of the Day: - Morning - Noon - Evening - Midnight Events always stop half a Day after they start. Time Manager has been designed to work with abrupt Time Jumps. Added Debug Messages for Event Starts and Stops. Added Name Parameter to CelestialEvent.
CelestialEvent now has 1 Constructor and Methods for changing other Values. Fixed Falling Dimando being impossible to happen. Added Spinning Dimando and Burning Dimando.
Events are now fully flexible with Start and End Time of Day. Events are still updated correctly when Time Jumps happen. It is now possible to make Events last for multiple Days. Added Time Machine to quickly change Time. Added Enumeration for different Times of Day.
Added onActivation and onDeactivation to call Actions precisely at the Start and End of an Event. Made CelestialRegisterEvent extend WorldEvent to get Access to the World. Celestial Events can now directly access the World. Moved the Posting of CelestialRegisterEvent into the WorldMixin. Added all Sorts of Changes to Game Mechanics for the Test Events. Moved some Debug Prints into a Test Mod Class. Started Work on a Saving System for the Activity State, currently unfinished.
Celestial Events now save and load their Activity Status from NBT, meaning that their Activity gets preserved after closing the Game. Added Initialization Logic to ensure correct Loading of NBT Data when the Game is started.
Fixed incorrect Loading for Edge Cases. Fixed Activation Method not properly saving the Activity State. Added State for attempted Activation to ensure that Events do not activate again when it already was attempted in the valid Time Frame. Event Registering and Time Management now automatically clean themselves up.
Documented all important API Components down to the Details. Introduced Lombok Getters. Added NBT Data Updating to the stopEvent() Method in CelestialEvent. Removed some redundant Code. Added Newline to CelestialRegisterEvent as requested.
Moved Initializing Process to CelestialInitializer. All Events are automatically added to it. Now Events without a Time Manager are initialized properly.
Events can now have Dependencies. Events only activate if their Dependencies are active. Fixed a Bug that caused attempted Activations to never be reset, leading to Events never happening again.
# Conflicts: # src/test/java/net/modificationstation/sltest/block/Blocks.java # src/test/java/net/modificationstation/sltest/item/ItemListener.java # src/test/resources/fabric.mod.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review. Got some thoughts on the overall design of the API.
...events-v0/src/main/java/net/modificationstation/stationapi/api/celestial/CelestialEvent.java
Outdated
Show resolved
Hide resolved
...events-v0/src/main/java/net/modificationstation/stationapi/api/celestial/CelestialEvent.java
Outdated
Show resolved
Hide resolved
* It is recommended to add the event to CelestialTimeManger for automatic management. | ||
* Inheritance is possible to add custom logic. | ||
*/ | ||
public class CelestialEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best name, considering Event
suffix most likely indicates an UnsafeEvents event. Not sure what would be a good alternative though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could replace the suffix with Interval
which might actually be a better hint for what it is
...events-v0/src/main/java/net/modificationstation/stationapi/api/celestial/CelestialEvent.java
Outdated
Show resolved
Hide resolved
Got my workspace fixed up, now I can continue work on the API |
The Method CelestialEvent.initializeEvent had a Parameter that would be declared within the Method anyway.
Only thing left as far as I'm concerned is to detach event registering from world load and instead moving it to game init. |
I am glad this PR is being picked up again. Thank you for putting in the time and effort to get the biggest flaws sorted out |
The Celestial Events API now has all the primary features needed to be considered functional. There is also a handful of different example usages in the test mod. This should be ready to be included into Station API