Skip to content
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

Added firemaking plugin for Kotlin #349

Open
wants to merge 7 commits into
base: kotlin-experiments
Choose a base branch
from

Conversation

tlf30
Copy link
Contributor

@tlf30 tlf30 commented Sep 10, 2017

Hello.
I have ported the firemaking plugin as discussed in #303. This has several fixes in it and I did test it.
The only things I am not sure on are the random equations for a successful chance of lighting the fire and how long the fire should burn.

EDIT: Also, this does not support lighting logs already on the ground because I have no way to test easily until dropping items is implemented. Also does not enforce not lighting fires in banks as I am not sure how to implement that.

Can someone please review.

PS: Yes, I know I have a lot of pull requests open right now. Sorry.

Thank you,
Trevor

}

fun walkCoords(direction: Direction): Position {
if (direction == Direction.NORTH) {
Copy link
Contributor

@garyttierney garyttierney Sep 10, 2017

Choose a reason for hiding this comment

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

position.step(1, direction))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is much simpler, thank you!

@garyttierney
Copy link
Contributor

PS: Yes, I know I have a lot of pull requests open right now. Sorry.

No, that's nothing to be sorry about. It's great :-). Will get around to reviewing shortly.

@tlf30 tlf30 force-pushed the kotlin-experiments-firemaking branch from 927c273 to 15ba396 Compare September 17, 2017 00:08
@tlf30
Copy link
Contributor Author

tlf30 commented Sep 17, 2017

This has been rebased and setup for the new build system for kotlin plugins.

Fix wrong class path
}


on { ItemOnItemMessage::class }
Copy link
Contributor

Choose a reason for hiding this comment

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

@Major- didn't you write something in Ruby to handle these ItemOnItem pairs?

Copy link
Member

Choose a reason for hiding this comment

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

I removed it because it wasn't very good, maybe we could look into something for kotlin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the ItemOnItemMessage not the correct way to do this?

Copy link
Member

@Major- Major- Sep 17, 2017

Choose a reason for hiding this comment

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

No this is correct, we're just discussing how we can make it a bit nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok

@Major- Major- added the blocked Blocked on another issue label Sep 18, 2017
@Major-
Copy link
Member

Major- commented Sep 18, 2017

Blocked on item dropping

@Major-
Copy link
Member

Major- commented Sep 24, 2017

After Firemaking is merged we're going to freeze kotlin-experiments for new content until it reaches master (i.e. we'll only be merging plugin ports, not new plugins). Any new content is welcome, but it will sit in PR until we're ready or stuff we still need to port will never get done.

@Major- Major- added plugin and removed kotlin labels Sep 24, 2017
@rmcmk
Copy link
Contributor

rmcmk commented Sep 30, 2017

@tlf30 I'm working on the ground item code now.

@tlf30
Copy link
Contributor Author

tlf30 commented Nov 15, 2017

@ryleykimmel your ground item code is awesome! Once it is merged in I update this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on another issue plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants