feat: add per-player interval mode (configurable) for goal evaluation#29
feat: add per-player interval mode (configurable) for goal evaluation#29Bonziman wants to merge 1 commit into
Conversation
…layer instead of globally
TheGaBr0
left a comment
TheGaBr0
left a comment
There was a problem hiding this comment.
I need you to change the target branch to dev and rebase this on it (you don't have to open a new PR in case you didn't know)
The PR is missing the automatic update for all existing goals
(see UpdateManager class for understanding the pattern) which requires a new config version for triggering the update (CURRENT_CONFIG_VERSION in main and config-version in config.yml) and a new class that will take care of the update (see Version364to365Updater class). You can add the goals update function like so:
private final GoalsManager goalsManager = GoalsManager.getInstance();
PlaytimeFormatsConfiguration playtimeFormatsConfiguration = PlaytimeFormatsConfiguration.getInstance();
playtimeFormatsConfiguration.initialize(plugin);
goalsManager.initialize(plugin);
goalsManager.goalsUpdater();
GoalsManager.resetInstance();
PlaytimeFormatsConfiguration must be initialized first as goalsUpdater triggers saveToFile which calls ticksToFormattedPlaytime, which depends on it.
| return Timestamp.from(instant); | ||
| } | ||
|
|
||
| private Instant instantFromDatabase(ResultSet rs, String columnName) throws SQLException { |
There was a problem hiding this comment.
I don't get what you were trying to achieve in the GoalsDAO class since this and getLastCompletionTime are both unusued, maybe something like: compute the next interval from last completion time, not from total playtime milestones?
This needs to be clarified
|
|
||
| private boolean perPlayerInterval; | ||
| private final Map<String, BukkitTask> perPlayerCheckTasks = new ConcurrentHashMap<>(); | ||
| private final Map<String, Long> targetPlaytimeByUser = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Correct me if you're implementation is based entirely and only on playtime requirements (which has to be explicitly clarified)
This is a pretty specific use case where the requirement is simply the playtime mapped per player (and works for you). It's probably better tho to implement it generally using the class GoalRewardRequirement.
| private final Map<String, Long> targetPlaytimeByUser = new ConcurrentHashMap<>(); | |
| private final Map<String, GoalRewardRequirement> targetRequirementsByUser = new ConcurrentHashMap<>(); |
| private boolean perPlayerInterval; | ||
| private final Map<String, BukkitTask> perPlayerCheckTasks = new ConcurrentHashMap<>(); | ||
| private final Map<String, Long> targetPlaytimeByUser = new ConcurrentHashMap<>(); | ||
| private final Map<String, Integer> completionsByUser = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
This is currently not used, what is its purpose?
| private final Map<String, BukkitTask> perPlayerCheckTasks = new ConcurrentHashMap<>(); | ||
| private final Map<String, Long> targetPlaytimeByUser = new ConcurrentHashMap<>(); | ||
| private final Map<String, Integer> completionsByUser = new ConcurrentHashMap<>(); | ||
| private final Set<String> lastCompletionLoading = ConcurrentHashMap.newKeySet(); |
There was a problem hiding this comment.
same here as the previous comment
| "per-player-interval:", | ||
| " - If true, the completion check interval is applied per player instead of globally.", | ||
| " - Only applies when check-time-interval is a number of seconds (not cron).", | ||
| " - Cron expressions always run globally.", |
There was a problem hiding this comment.
Cron expressions always run globally.
Yep nice idea
|
|
||
|
|
||
|
|
||
| private void recordCompletion(DBUser user) { |
There was a problem hiding this comment.
What would be the use of this? To record the goal completion in the db?
Which is already done by the async markGoalAsCompletedAsync which calls this method subsequently.
| if (!canRunPerPlayerCheck(user)) { | ||
| return; | ||
| } | ||
| checkCompletion(user); |
There was a problem hiding this comment.
Be careful here.
checkCompletion checks for all kind of requirements, not only playtime.
This links back to my previous comment (line R62)
| } | ||
|
|
||
| public void handlePlayerJoin(OnlineUser user) { | ||
| if (!active || !perPlayerInterval || useCronExpression) { |
There was a problem hiding this comment.
!active is redundant since schedulePerPlayerCheck already checks for it
|
|
||
| if (perPlayerInterval && !useCronExpression) { | ||
| scheduleInfo.put("nextCheck", "per-player"); | ||
| scheduleInfo.put("timeRemaining", "per-player"); |
There was a problem hiding this comment.
| scheduleInfo.put("timeRemaining", "per-player"); | |
| scheduleInfo.put("timeRemaining", checkTimeToText); |
| Map<String, Object> scheduleInfo = new HashMap<>(); | ||
|
|
||
| if (perPlayerInterval && !useCronExpression) { | ||
| scheduleInfo.put("nextCheck", "per-player"); |
There was a problem hiding this comment.
| scheduleInfo.put("nextCheck", "per-player"); | |
| scheduleInfo.put("nextCheck", "each player's playtime milestone"); |
2 participants
This PR adds an optional configuration to enable per-player interval execution for goal checking.
Changes:
Why:
Some servers may prefer per-player scheduling for more accurate and isolated goal tracking, while others may prefer the existing global interval system for performance reasons or for server wide goals.
This change keeps backward compatibility and does not modify default behavior.
Notes: