Skip to content

Spiders den rain notifications #1069

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

darkkeks
Copy link
Contributor

@darkkeks darkkeks commented Dec 7, 2024

Implementation of #1068. Could not find the code for /api/calendar endpoint, so I added calculation to the mod itself, since the logic is quite simple. Also the API seems to return only old events now and I can't test that patch does not break existing code :(

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Dec 7, 2024
@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label Dec 7, 2024
@AzureAaron AzureAaron added the merge conflicts This PR has merge conflicts that need solving. label Dec 22, 2024
@LifeIsAParadox LifeIsAParadox removed the reviews needed This PR needs reviews label Feb 15, 2025
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr needs to be updated. I've also left some comments that can be addressed while updating.

@@ -85,10 +120,18 @@ public static void init() {
));
}

private static final Map<String, LinkedList<SkyblockEvent>> events = new ConcurrentHashMap<>();
private static final AtomicReference<Map<String, LinkedList<SkyblockEvent>>> events = new AtomicReference<>(Map.of());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need an atomic reference here. A non-final field will do.

Comment on lines +85 to +91
private static void register() {
if (initialized) {
return;
}
Scheduler.INSTANCE.scheduleCyclic(EventNotifications::refreshEvents, 20 * (int) EVENTS_UPDATE_PERIOD.toSeconds());
initialized = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more complicated than necessary. Just put this in init().

Comment on lines +129 to +134
public static IntList getDefaultReminders(String eventName) {
var periodicEvent = PERIODIC_EVENTS.stream().filter(event -> event.name.equals(eventName)).findFirst();
if (periodicEvent.isPresent()) {
return periodicEvent.get().defaultReminders();
}
return DEFAULT_REMINDERS;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this above getEvents.

Comment on lines +169 to +180
private static void addFixedEvents(long nowSeconds, Map<String, LinkedList<SkyblockEvent>> events) {
PERIODIC_EVENTS.forEach(event -> {
long firstEvent = SkyblockTime.SKYBLOCK_EPOCH / 1000 + event.offsetFromSkyblockEpochSeconds;
long currentEvent = firstEvent + (nowSeconds - firstEvent + event.periodSeconds - 1) / event.periodSeconds * event.periodSeconds;
// Prepare events for next two update periods in case one is skipped for some reason, or at least one if event is rare.
long eventsToAdd = Math.max(1, 2 * EVENTS_UPDATE_PERIOD.toSeconds() / event.periodSeconds);
events.put(event.name, LongStream.range(0, eventsToAdd)
.map(i -> currentEvent + i * event.periodSeconds)
.mapToObj(ts -> new SkyblockEvent(event.name, ts, event.durationSeconds, new String[0], event.warpCommand))
.collect(Collectors.toCollection(LinkedList::new)));
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda (passed into forEach) could be turned into a method in SkyblockerPeriodicEvent. Also, please add some javadoc for what this method does, as the name add could be unclear.

Map<String, LinkedList<SkyblockEvent>> newEvents = objects.stream()
.map(SkyblockEvent::of)
.filter(event -> event.start + event.duration >= nowSeconds)
.collect(Collectors.groupingBy(SkyblockEvent::name, Collectors.toCollection(LinkedList::new)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we need linkedlist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflicts This PR has merge conflicts that need solving. new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional notifications for Spider's Den rain and thunderstorm
4 participants