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

QoL Changes to Sword Sheaths + Katana Sakuride Coating #16383

Merged
merged 11 commits into from Oct 18, 2023

Conversation

KevinfromHP
Copy link
Contributor

@KevinfromHP KevinfromHP commented Oct 10, 2023

About the PR

  • All sword sheaths can now be interacted with in a more comfortable manner. You can now mouse drag to remove the sheath like any other bag.
  • New feature: all katanas can now be coated with sakuride! Clicking on a pure beaker of sakuride will coat the katana in up to 15u of it. When you use your dash, it will now leave a trail of sakura petals behind you. Each tile uses one unit, so you have 5 dashes before it's used up. As of now, you can only coat it in sakuride, and it has no effect beyond this.

Why's this needed?

  • The katana can be a pain in the ass to work with. The control changes felt like a necessity since you would have to take the sword out, then use you other hand to remove the sheath.
  • The sakuride coating feels like a nice touch to the katanas. I just thought it'd be fun.

Changelog

(u) Kevin From HP
(+) Sword sheaths can now be dragged out of belt slot onto hand/ground.
(+) Using any katana on a container of pure sakuride will coat it in up to 15u. Dashing leaves a trail of petals behind you.

…dojo

Created compatibility to initialize sword sheaths without a sword inside of them.
Also replaced one of the unusable katana sheaths in the dojo to an empty useable one (nice to have for the craftable katana)
@boring-cyborg boring-cyborg bot added the A-Mapping A mapping change label Oct 10, 2023
@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 10, 2023
@DisturbHerb
Copy link
Contributor

Don't think the workshop katana needs a sheath, myself.

@CheffieGithub
Copy link
Contributor

CheffieGithub commented Oct 10, 2023

Its incredibly weak for the time investment so a sheath is a nice small buff. Maybe some distintion from the normal katana sheath is a good idea though.

@CheffieGithub
Copy link
Contributor

Personally intent based interactions feel bad but we don't really have a framework for other shortcuts as alternatives

@KevinfromHP
Copy link
Contributor Author

KevinfromHP commented Oct 10, 2023

I can drop the grab intent thing if that's what's preferred, I personally like how the shinai bag's intent changes things but if that's not a popular thing then that's alright. As for the sheath, if you're looking to use it for anything other than aesthetic then I feel like you'd be wasting your time. A strangelet loaf or even a loaf a few tiers lower is a greater return on time investment than that katana, if someone's gonna spend so much time on it I feel like they might as well get a place to put it.

@CheffieGithub
Copy link
Contributor

Grab is fine but maybe being able to drag click it into your hands would be good as well

@KevinfromHP
Copy link
Contributor Author

Grab is fine but maybe being able to drag click it into your hands would be good as well

Already done, same as a backpack/belt

Copy link
Contributor

@TobleroneSwordfish TobleroneSwordfish left a comment

Choose a reason for hiding this comment

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

  • Click dragging is good, but intent based inventory interactions are very unclear.
  • The workshop katana not having a sheath is very intentional.
  • The sakura petals are cute and cool 👍

@zjdtmkhzt
Copy link
Contributor

What's the intent with the workshop not having a sheath? I feel like it would be much cooler if it did, katana without a sheath kind of just sucks from a coolness standpoint.

I agree though that it should probably have a different name and icon to make it so you do not appear to be an antagonist?

@KevinfromHP
Copy link
Contributor Author

KevinfromHP commented Oct 13, 2023

I considered a new sheath, but already being able to make both a handcrafted and reverse blade (which already has a sheath) there made me think it was best to go with a vanilla sheath.

I can drop grab intents easy enough, but if it were to receive its own unique sheath, could that change be kept?

@DisturbHerb
Copy link
Contributor

I think being able to wear and draw a katana at will is something best left to the traitor katana. That's what I assumed the reasoning to be against providing sheaths for the workshop ones.

@KevinfromHP
Copy link
Contributor Author

Though I disagree due to the lack of power of the katana(s) compared to other options, I'll drop the sheath.
However, I will leave my final thoughts on it. Reading over the discussion in the discord over this, I do not think the sheath would make a significant change to the way people play given the time it would take to create, a traitor's instant access to a better katana with a sheath, and the nature of "power gaming".
It surprised me how controversial this is considering many of the other options that are available, like the (storable) predator revolver and rifle on ice moon, a scrap plasma dagger, and the 120 seconds or less to obtain phaser in debris field. If someone were interested in a viable weapon, the 20-30 minute katana from the workshop seems illogical unless it was solely an aesthetic choice. I wouldn't see this as a major change to the game though people might see a katana slightly more often if it had a sheath.

@KevinfromHP KevinfromHP marked this pull request as draft October 14, 2023 03:50
@KevinfromHP
Copy link
Contributor Author

  • Click dragging is good, but intent based inventory interactions are very unclear.
  • The workshop katana not having a sheath is very intentional.

These two things have been revised as requested

@KevinfromHP KevinfromHP marked this pull request as ready for review October 14, 2023 04:04
code/obj/item/misc_weapons.dm Outdated Show resolved Hide resolved
_std/defines/item.dm Outdated Show resolved Hide resolved
code/obj/item/misc_weapons.dm Outdated Show resolved Hide resolved
code/obj/item/misc_weapons.dm Outdated Show resolved Hide resolved
Co-authored-by: TobleroneSwordfish <20713227+TobleroneSwordfish@users.noreply.github.com>
@Azrun
Copy link
Contributor

Azrun commented Oct 15, 2023

I think this PR is doing far too many things. Sakura should be atomized to a separate PR.

@KevinfromHP
Copy link
Contributor Author

KevinfromHP commented Oct 15, 2023

I think this PR is doing far too many things. Sakura should be atomized to a separate PR.

If you removed sakuride coating from this PR, wouldn't it just be making the sword sheath mouse draggable?

The only other thing it has is the ability to spawn sheaths in map data but that's just an artifact from an old change to add a sheath to hidden workshop, I can drop that once I finish these changes

• Removes KATANA_REAGENT_CAPACITY global
• Informs the user when the coating is already applied
• Informs the user when the coating cannot be used at all
• Informs the user when the coating is fully used
• Better better coating transfer
@KevinfromHP KevinfromHP changed the title QoL Changes to Sword Sheaths + Other Katana Changes QoL Changes to Sword Sheaths + Katana Sakuride Coating Oct 15, 2023
Copy link
Contributor

@TobleroneSwordfish TobleroneSwordfish left a comment

Choose a reason for hiding this comment

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

This ideally should be atomized, yes, but I'm fine with merging it as the changes are relatively minor. Just in future please don't package different changes into one PR.

code/obj/item/misc_weapons.dm Outdated Show resolved Hide resolved
code/obj/item/misc_weapons.dm Outdated Show resolved Hide resolved
@KevinfromHP
Copy link
Contributor Author

That commit says checks if human but it checks if it's a mob, my mistake

@TobleroneSwordfish TobleroneSwordfish merged commit c4a5b4b into goonstation:master Oct 18, 2023
21 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Mapping A mapping change size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants