Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds GameObjectDespawned event handling to Rs2SpiritTreeCache, delegating to the existing strategy. In SpiritTreeUpdateStrategy, expands handled events to include GameObjectSpawned/Despawned, adds a private handler to update cache entries for POH spirit trees based on object presence, and removes the previous POH-specific update path and related POH gating from varbit/game-state handling. Updates imports and object ID sets to include POH_SPIRIT_TREE. In SpiritTree, sets POH entries’ requiredSkillLevel to 0, switches POH_SPIRIT_RING IDs to Rs2GameObject lookup, and removes POH-specific checks from isAvailableForTravel(). Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| return isPatchHealthyAndGrown(); | ||
| } | ||
|
|
||
| // Check construction level for POH trees |
There was a problem hiding this comment.
Can you elaborate why you removed this? I'm not fully understanding this change.
There was a problem hiding this comment.
I removed it from the function isAvailableForTravel(). Your construction or farming levels have nothing to do with whether you can use a tree for travel. It either exists or it doesn't. And when it does, you can use it.
There are many people that boost their farming/construction levels to plant the tree. As such, this lead to unintended blocking behavior.
Additional rant:
I would even go as far as saying that all the level requirements in this file should be removed as planting spirit trees comes with higher level requirements the more trees you plant. "At level 83 Farming, players can only have one spirit tree planted at a time, not including the one in their player-owned house. Level 91 Farming allows for two spirit trees to be planted at a time, whilst 99 Farming allows for unlimited spirit trees." But -to make things more complex- the tree in your PoH does not count towards this. "Players can plant their own spirit tree from a spirit sapling in their Superior Garden at level 75 Construction. Doing so also requires level 83 in Farming. Boosts can be used. This tree does not count towards the number of spirit trees that may be planted at one time."
There is also a hasLevelRequirement() function where the level is being looked at, but that doesn't consider the amount of trees you have and uses Rs2Player.getRealSkillLevel(Skill.FARMING) >= requiredSkillLevel instead of getBoostedSkillLevel(). None of this code is used anywhere. If it would be used for actual farming, it wouldn't work.
There was a problem hiding this comment.
I removed it from the function
isAvailableForTravel(). Your construction or farming levels have nothing to do with whether you can use a tree for travel. It either exists or it doesn't. And when it does, you can use it. There are many people that boost their farming/construction levels to plant the tree. As such, this lead to unintended blocking behavior.Additional rant: I would even go as far as saying that all the level requirements in this file should be removed as planting spirit trees comes with higher level requirements the more trees you plant. "At level 83 Farming, players can only have one spirit tree planted at a time, not including the one in their player-owned house. Level 91 Farming allows for two spirit trees to be planted at a time, whilst 99 Farming allows for unlimited spirit trees." But -to make things more complex- the tree in your PoH does not count towards this. "Players can plant their own spirit tree from a spirit sapling in their Superior Garden at level 75 Construction. Doing so also requires level 83 in Farming. Boosts can be used. This tree does not count towards the number of spirit trees that may be planted at one time."
There is also a
hasLevelRequirement()function where the level is being looked at, but that doesn't consider the amount of trees you have and usesRs2Player.getRealSkillLevel(Skill.FARMING) >= requiredSkillLevelinstead of getBoostedSkillLevel(). None of this code is used anywhere. If it would be used for actual farming, it wouldn't work.
Makes sense
| 75, // Requires 75 Construction | ||
| 95, // Requires 95 Construction | ||
| VarbitID.POH_SPIRIT_TREE_UPROOTED, // TODO Must be checkedPOH Spirit Tree varbit (combined tree/ring uses same varbit) | ||
| List.of(ObjectID.POH_SPIRIT_RING), // TODO Must be checked Spirit Ring object ID // we must update it. here are also variations for it.. leauge skins |
There was a problem hiding this comment.
Why use Rs2GameObject here and not the actual IDs from ObjectID? Not using the variations and also only the spirit tree variation with the fairy ring.
only using "poh_spirit_ring" is the wrong approach here. - is it not only the spirit tree with the fairy ring? What about the spirit tree without the fairy ring? Here we must consider not only https://oldschool.runescape.wiki/w/Spirit_tree_%26_fairy_ring, but we should add all variations for https://oldschool.runescape.wiki/w/Spirit_tree_(Construction) and the Spirit tree & fairy ring.https://oldschool.runescape.wiki/w/Spirit_tree_(Construction) -> why is the construction level changed to 95?
https://oldschool.runescape.wiki/w/Superior_garden
There was a problem hiding this comment.
Why use Rs2GameObject here and not the actual IDs from ObjectID? Not using the variations and also only the spirit tree variation with the fairy ring.
Well, the Rs2GameObjects.getObjectIdsByName function returns a list of all IDS we're interested in, which is exactly what you also want I believe?
There was a problem hiding this comment.
The requirement for Spirit tree with fairy rings is 95 construction: https://oldschool.runescape.wiki/w/Spirit_tree_%26_fairy_ring
There was a problem hiding this comment.
i see why it better to use the reflection approach here, because of the different IDs per configuration, great job on the changes otherwise.
Regarding the level. Sorry, my bad, have not seen the other one.
There was a problem hiding this comment.
i see why it better to use the reflection approach here, because of the different IDs per configuration, great job on the changes otherwise. Regarding the level. Sorry, my bad, have not seen the other one.
Aah I understand your confusion. It is unclear from the diff that I am changing only the Spirit Rings, not Spirit Tree enum value.
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Why remove these functions entirely, and why can't we update the state based on the varbit for the spirit tree?We must wait for the object cache check to complete the scan and have populated the update. In the handleGameStateChanged, you cannot use the cache because it's not updated, so we would need to wait for the object cache to be updated on the object spawn and despawn events, also on a gamestate change event.
There was a problem hiding this comment.
You can use the varbit still. I didn't touch the part for POH spirit tree varbits that you left with //TODO to still be implemented:
if(varbitId == VarbitID.POH_SPIRIT_TREE_UPROOTED){
log.debug("TODO update cache for POH spirit tree uprooted varbit change,currently in POH? {} ",PohTeleports.isInHouse());
}What I removed from handleVarbitChange was just a proxy for scanning for gameobjects:
if (Rs2Cache.isInPOH()) {
log.debug("Player in POH, checking for spirit tree objects,changed varbit {}", varbitId);
updatePOHSpiritTreeCache(cache);
}In the handleGameStateChanged, you cannot use the cache because it's not updated, so we would need to wait for the object cache to be updated on the object spawn and despawn events, also on a gamestate change event.
This is what I gave as a reason to remove scanning for objects in POH from the handleGameStateChanged function. I'm not sure what you're saying here exaclty.
There was a problem hiding this comment.
You have to delay the scan, of course, until the data are stable. This is what I'm referring to.
There was a problem hiding this comment.
You have to delay the scan, of course, until the data are stable. This is what I'm referring to.
Yes that would be an option. How long are you going to delay it for? Until the game object spawns preferably, right? So how about we just look at the gameobjectspawns event instead and skip the delaying setup altogether?
3 participants
isAvailableForTravel()as it is not a requirement to use the spirit tree