-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: fishing skill #390
base: develop
Are you sure you want to change the base?
feat: fishing skill #390
Conversation
Co-authored-by: James Monger <[email protected]>
22f2929
to
d18beee
Compare
still in progress, will work with @jameshallam93 |
return; | ||
} | ||
if(taskIteration === 0) { | ||
this.actor.sendMessage('You swing your axe at the tree.'); |
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.
Whoops
// TODO (jameskmonger) this doesn't currently account for a moving NPC target | ||
targetActor.position, |
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.
we should change ActorWalkToTask
to also accept Actor
rather than passing the position in manually, that way we can easily defer the "tracking" of moving targets easily.
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.
then this would just be
// TODO (jameskmonger) this doesn't currently account for a moving NPC target | |
targetActor.position, | |
targetActor, |
return; | ||
} | ||
|
||
// TODO: check npc still exists |
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.
need to investigate what happens when NPCs die/are removed
I suspect we need to do something similar to the activeWorld.findObjectAtLocation
in ActorLandscapeObjectInteractionTask
@@ -12,6 +12,10 @@ import { ActorWalkToTask } from './actor-walk-to-task'; | |||
* @author jameskmonger | |||
*/ | |||
export abstract class ActorLandscapeObjectInteractionTask<TActor extends Actor = Actor> extends ActorWalkToTask<TActor, LandscapeObject> { | |||
/* | |||
* TODO (jameskmonger) consider exposing this, currently people must always access it through `otherActor` |
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.
* TODO (jameskmonger) consider exposing this, currently people must always access it through `otherActor` | |
* TODO (jameskmonger) consider exposing this, currently people must always access it through `landscapeObject` |
my bad
import { canInitiateHarvest } from '@engine/world/skill-util/harvest-skill'; | ||
import { randomBetween } from '@engine/util'; | ||
|
||
|
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.
please add javadoc comment
const roll = randomBetween(1, 256) | ||
if(roll > 200){ |
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.
we should move this into a separate file (see woodcutting, which has its own chance
file) though also some research is needed on the OSRS wiki to see how fishing chances are handled
Co-authored-by: James Monger [email protected]