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

Increase crafting distance to 6. #5279

Merged

Conversation

kevingranade
Copy link
Member

Allows pulling components and tools from a room-sized area when crafting instead of just the immediate vicinity.

@infectedmochi
Copy link
Contributor

Increased distance is nice, but will this enable player character to craft with components from the other room when locked in? Are there any mechanic in place to prevent this exploit?

@atomicdryad
Copy link
Contributor

This is addressed by #4904. Increasing the size to an arbritrary number does not simulate shelf/rack functionality or provide a reason for the players to make them.

@BevapDin
Copy link
Contributor

@infectedmochi: yes inventory::form_from_map requires a map::clear_path to the tile to take items from there, so items from closed of areas are not added to the crafting inventory.

@Rivet-the-Zombie
Copy link
Member

I'm in favor of this, but I'm also still in favor of #4904.

@SilearFlare
Copy link

6 is a little too much, don't you think?Does the character have extremely stretchy arms? I'm perfectly fine with 2.
And yeah, #4904 would be the best. I remember it working for one of the nighties, and then nothing.What happened?

@Killer-of-Lawyers
Copy link

@tyrael93 It had a bug that broke a lot of crafting in it, so it had to be pulled. Once it was fixed no one pulled it back in.

@Rivet-the-Zombie
Copy link
Member

6 is a little too much, don't you think?Does the character have extremely stretchy arms? I'm perfectly fine with 2.

Stretchy arms? Is it really asking too much to assume that the hero's feet aren't nailed to the floor while crafting?

Like, in the time that they spent crafting perhaps they were bright enough to move around a little and pick up the necessary materials?

@Zireael07
Copy link
Contributor

Crafting distance 6, just because we can?

NO.

Pull #4904 in and we'll have the same thing but in a more intuitive and logical way.

@kevingranade
Copy link
Member Author

The only reason #4904 is still open is in case someone proposes a change to it that addresses the problems I have with it. As it is it's not going in, I've been extremely clear about this at every step of the process.

@Killer-of-Lawyers
Copy link

You're the only one with a problem. Are you going to stop everything that the community wants if it doesn't fit your view? That sort of defeats the community aspect of it all.

@kevingranade
Copy link
Member Author

This is very much a community project, but the final decision is mine, not vote or consensus or what have you. It's my decision because I'm doing a huge amount of work, anyone who feels like stepping up and taking on that workload is free to make a fork.
I very rarely refuse a working contribution, but in my opinion #4904 is the wrong fix for a monstly nonexistent issue. This addresses the heart of the issue without a complicated and nonsensical special-casing of crafting/furniture interactions.

@Killer-of-Lawyers
Copy link

Then why didn't you do this the first time the request was up, before it was pulled. Before the author of the request fixed the bug that was with it then and resubmitted it? Why didn't you just plainly say no and instead drag things out into over a month of debate?

@kevingranade
Copy link
Member Author

Oi, cut the hostile tone. I said it wasn't ok on IRC before there was an implementation at all, I reiterated it in the PR at #4904 (comment), again at #4904 (comment). It's not my fault if people state that they disagree with my rationale and then ignore it. No new argument has come up, so I'm not going to keep arguing the same point repeatedly. As for the first PR, AD merged it before I had a chance to comment on it, and then I had to revert it.

@infectedmochi
Copy link
Contributor

While I'm in favor of #4904 getting merged, I just want to say that I'm perfectly fine if we work together and find a middle-ground, rather than bickering among ourselves and not getting anything done. Constructive debate is good, but there a fine line between a constructive and a meaningless one.

@atomicdryad
Copy link
Contributor

So is Cataclysm Kevin Granade's game, or a community effort? Because I was hoping to Gryph would have gotten a chance to give feedback on #4904, however it was closed it before he could do so.

Thus far, the only issue with #4904 was that one dev would/could not accept how it made sense, and did not care that everyone who bothered to comment wanted that feature.

This PR only addresses one of the reasons why #4904 was a dammed good contribution.

@Eighth-of-Third
Copy link
Contributor

I have to chime in on this as well. I would much prefer to have #4904 merged instead of this one, for most of the reasons already mentioned by other people.

This PR is simply an inferior attempt to fix a single one of the problems #4904 addresses.

@Killer-of-Lawyers
Copy link

My tone is not hostile. As much as text can have a tone anyways. What I said was if this is your game, and you didn't want input, you should have been more clear about it, instead of waiting for months to do this. I was under the impression that this was not the case, and I wish I knew it in the past.

@Turtlicious
Copy link
Contributor

I find it kind of weird that one person is rail-roading the project. I thought this was a community project? I'm kind of pissed because I donated for Cataclysm: Dark Days Ahead, not Kevin Granade's Cataclysm: Dark Days Ahead.

"It's a community project, except only I get to say what goes in." is a bit disingenuous. That makes it not a community project, it makes it a project where other people do work for you.

@SilearFlare
Copy link

Kev, you're really digging your grave here. There's only one thing you can do to save your face, revert this and enable #4904 . Everyone loves that, nobody loves this except for you.
It ain't that difficult, mate.

also: kevingranade commented a day ago

"Furniture manipulating crafting distance simply doesn't make any sense."
That's the "reason" you're using? That's the only thing you could come up? Goddamnit kevin, merge the damn thing already, this isn't a tiranny it's a goddamn COMMUNITY PROJECT. Deal with the fact that there are people that like things that you don't like (almost everyone in this case).

@Nickboom1
Copy link
Contributor

Guys chill out. I agree that it should be merged but y'all should be nicer to Kevin if it wasnt for him and the other core devs we wouldn't have DDA. I say Kevin's opinion matters a great deal however if many people want it in the game and people dont seem to have a problem with it then why veto it. If we add it and people like it thats good but if people dont like it then we can simply remove it from the game.

Also Killer-of-lawyers your not helping very much your just adding fuel to a fire your comments arent even really about the damn thing you want merged.

@SilearFlare
Copy link

I suppose my tones were a little harsh too, but my point still stands. As I said in #4904 too, you can either let this slide, or if you think it's worth it make a poll for all the devs working on the project. Yes or no to merging this.
If yes gets the majority, pull it.
If no gets the majority, don't pull it.
I'm sure you can get on skype or something and just say "I'm against this" or "I want this". I wouldn't let a non-dev decide, but that's still up to you guys.

This is the simplest and most reasonable solution, in my humble opinion.

@Killer-of-Lawyers
Copy link

I'm not adding fuel to any fire unless people are wanting to be fired up. I'm just trying to confirm if this is Kevin's show or not, and that's a valid thing to ask.

@KA101
Copy link
Contributor

KA101 commented Jan 3, 2014

Just a heads-up, Kevin's starting his new job in Seattle so will likely be busy for a few days. He said as much on the IRC last night.

Kevin's the lead merger; along with GlyphGryph (the repo owner, forum admin, and Kickstarter head) he's first among equals so far as I know.

(Asking to poll the devs is tricky. Define dev: Anyone with something merged? Anyone with X items merged, or X items over Y time? Anyone with merge authority? Anyone with Contributor or Admin status on the forums?)

@SilearFlare
Copy link

(Asking to poll the devs is tricky. Define dev: Anyone with something merged? Anyone with X items merged, or X items over Y time? Anyone with merge authority? Anyone with Contributor or Admin status on the forums?)

I think anyone with merge authority should have a vote, but not be forced to vote, he/she just has the possibility to do so.

Just open a new issue called "Voting poll for #4904 and #5279 ".

@kevingranade
Copy link
Member Author

I thought my previous posts were sufficiently clear, but I guess not.
It is in fact "my show". I have final say on what goes in, it's just that
the vast majority of the time* I don't need to make a call because we reach
a consensus readily.
I'm rather surprised that this comes as a shock to anyone, the vast
majority of open source projects, including some of the largest software
projects in existence are run like this.
Again, if someone thinks they can do a better job, they're free to try by
forking the source, but ordering me around or trying to impose some
committee on me simply isn't going to work, because I'm just going to
ignore you. If you want to discuss issues I'm all ears, but there hasn't
been any substantative discussion on this** one for about a month***.

_I've had to make a hard call literally a handful of times, vs over 10,000
commits accepted by consensus.
*_By which I mean the PR, not the discussion about project governance.
***"I agree" is not substantative discussion.

@Eighth-of-Third
Copy link
Contributor

There's been no substantive discussion on this for a month because a consensus has already been reached a month ago, albeit one that does not include you. Consensus means general agreement, not unanimous agreement.

Quite simply, literally everyone who has bothered to comment thinks that #4904 is a good commit and should be merged, except for you. There was quite a bit of discussion about how it encouraged base-building, helped with immersion, and gave furniture like counters or lockers a purpose other than flavour.

We're not trying to impose some sort of committee on you, but leaders do need to listen to what their people say.

@kevingranade
Copy link
Member Author

You aren't trying to tell me what to do, but I should do what you say, if
there's a distinction there I can't see it.
I did listen, I did discuss the issue, but in the end I disagree.
If at the end of that process I'm allowed to act on my own opinion, I'm a
leader, if I'm not, I'm a slave.
I'm not a slave.
ONE MORE TIME: There's a way for you to override me, and that way is for
you to do my job, but better, good luck.

@Eighth-of-Third
Copy link
Contributor

Are you serious? This is Github, Kevin, not Rapture. There's a difference between orders and friendly advice.

@SilearFlare
Copy link

You've got to be fucking kidding me, granade.
You're spitting in the eye of every donator.
I hope that before writing that last entry, you have either been drinking or smoking illegal substances that inflate your ego to the size of the Hinderburg, because you are not a leader, Granade. You're just a goddamn person sitting in front of a computer with unwarranted self importance trying to make his life a little less miserable. You're not a Leader. You're not a slave. You're nothing. You are just another nobody in the waste of the internet like me, gryph, atomic, everyone.

Your pathetic scream for your so called "freedom" which chains and binds everyone else to your will if they ever disagree with you, is nothing but a disgusting and twisted dictatorship, made by a man who is no more worthy than anyone else to direct a project like cataclysm.
I am speaking to you as a man to a man, not as a leader to a slave not as a leader to another leader, not as anything. Just as a nobody to a nobody.

We're nothing. You're nothing.

And this is just a videogame.

Go get some goddamn fresh air, get a cool drink and stop bitching.

@Lain-
Copy link
Contributor

Lain- commented Jan 4, 2014

And... this is getting ridiculous. People, would you please stop spamming github with images? I came here to see meaningful discussion and all I see was flamming.

@Nickboom1
Copy link
Contributor

Guys come on this is getting silly. All this over one PR.

@ghost
Copy link

ghost commented Jan 4, 2014

I know this is no longer the pull request discussion, but quick look in the code shows several potential issues caused by the use of PICKUP_RANGE in places other than just form_from_map calls:

  • PICKUP_RANGE is also used in consume_vpart_item which doesn't use m.clear_path, so with this change it'll be possible to install vehicle parts located behind walls etc. It's possible now, true, but with PICKUP_RANGE==2 it's much less likely to occur;
  • similar to the above, map::use_charges and similar don't check visibility, so technically a player can have a charged tool available near him, but use another, potentially inaccessible, tool;
  • and from the gameplay perspective - now you can have say two vehicles 10 cells apart, and a kitchen unit from the first vehicle can be used simultaneously with the second vehicle's chemistry lab in the same recipe. Granted there may be no such recipes at the moment and OBSTACLE'd parts will prevent that, but still looks a problem.

There are probably other similar issues as well, the above were all I could think of after looking in the code for 10 minutes. In general it seems the whole "inventory from map" thing is more or less broken, but this is not very noticeable with the current PICKUP_RANGE of 2. I'm pretty sure that setting it to 6 will cause a bunch of issues.

@KA101
Copy link
Contributor

KA101 commented Jan 4, 2014

@v-ti and THAT is good information to have. Thanks.

@Killer-of-Lawyers
Copy link

The only issue with open source is in the end of the day you'll still be a slave as long as you contribute. Your work will be merged and used, and what you don't like will still be in the fork. The only way to not be a slave would be to quit.

The only leader, if anyone, should be Glyph, since he's proven he won't babble about slavery and nonsense to protect his ego. I'm not even going to touch how terrible it is for a white male to be babbling about being a slave in this day and age.

@Sheco
Copy link
Contributor

Sheco commented Jan 4, 2014

While I agree with both sides, I think there should be an agreement between the "community" and the "developers".

Both solutions are equally flawed.

Kevin's solution is just a quick hack to "solve this issue" and focus on other goals, but it's too lazy and wouldn't feel good.

On the other hand, RabbitB's method of letting the container decide how craft-friendly it is doesn't really make sense.

A good middle ground might be being able to use whatever items are in sight without crossing the room's boundaries (doors and windows) nor exceeding the PICKUP_RANGE (6?). This would be like a range-limited "Look all items around the player" (V)

The player should be able to open/close lockers and other containers that don't let you see inside unless you're close to it.

There could be time penalty, adding the time it would normally take to walk to each item and back to the original place. (the crafting time should be shown before starting)

@kevingranade
Copy link
Member Author

Other than the time penalty, what you describe is what this does (modulo a
fix to the issues that were pointed out earlier, which I'm nearly done
addressing). The crafting code already checked that the player had a
direct path to the items.

@Sheco
Copy link
Contributor

Sheco commented Jan 4, 2014

Then I think there's nothing wrong with your solution.

@Sheco
Copy link
Contributor

Sheco commented Jan 4, 2014

Does it handle the containers that don't let you see inside from a distance?

@kevingranade
Copy link
Member Author

Currently it allows you to craft from "closed container" furniture (such as lockers) under the assumption that you can walk over and open them as needed.
@v-ti I added a unified method for checking whether the player can get at items in a given square, and have vehicle::vpart_consume_item(), map::use_amount() using it.
About the multi-vehicle issue, I really don't see that as any more of a concern than having two workstations far apart within the same vehicle, you can just run back and forth as needed to complete the crafting.

@i2amroy
Copy link
Contributor

i2amroy commented Jan 4, 2014

Ok, just gonna drop my two cents here.

  1. I liked the qualities system in [FIXED] Expanded Qualities System & Smart Crafting Distances #4904. I definitely would have merged that if it hadn't of been bundled with everything else.
  2. Furniture randomly warping space is kinda weird, and I agree with Kevingranade on that point. I do think that we need some sort of "organization bonus", which furniture could play a definite role in determining, but I don't think that simply having a flat "range:4" type field on every piece of furniture is the right way to go about it. Maybe have the option to "organize" a shelf, after which the range would be determined by how many items were present on the shelf, how much you had put recently on it, what type of furniture it is, etc (though steps would need to be taken to not just make more inventory management, as well as providing ample feedback of when something is in crafting range vs. out of it).

@Sheco
Copy link
Contributor

Sheco commented Jan 4, 2014

@i2amroy kevingranade has stated the qualities system is ok, but as it is right now it wouldn't be used anywhere else, maybe if there were other proposals it would be mergeable.

About the second point, the way kevin is doing it is quite simple and easy for the player, you spread your things around the room and it will find them.

I still think there should be a time penalty to simulate the player walking around manually getting the items, so you would organize items placing the common items closer to your "crafting bench", this would close the "how long are the character's hand, anyway?" argument.

@Turtlicious
Copy link
Contributor

I mean really, who /is/ John Galt?

@Lain-
Copy link
Contributor

Lain- commented Jan 4, 2014

+1 to the suggestion that it takes time to walk around and collect necessary items; especially when an enemy is near. Maybe just a small amount proportional to total distance the character has to walk.

@Zireael07
Copy link
Contributor

I think both this and #4904 have their merits. This one, coupled with a time penalty (especially when enemies present) is a good idea.

@ghost
Copy link

ghost commented Jan 4, 2014

@kevingranade Even with the additional fixes, at least the following broken case remains, as far as I can tell (I may be wrong here, but this is how it looks from the code):

  • place two vehicles let's say 6 cells apart. Both vehicles have welding rigs with charged accumulators;
  • stand in the first vehicle so that the second vehicle is still within 6 cells and passes the line-of-sight check, activate a crafting recipe that needs a welder;
  • now, the call to map::use_charges can take charges from either vehicle depending on their relative position. I think that most would agree that the first vehicle's (the one the player is standing in) accumulator should be used. And no, it's not a feature since the player has no control over this besides placing the vehicles in some way that only works because the code is written like this.

In any case, my point was not "here's a bunch of defects we get with this change, let's fix them and we're ready to go", but rather that the current implementation of "inventory from map" is broken, at least to some degree. This isn't noticeable now, when PICKUP_RANGE is just 2, but it will be when you increase it manifold. I think the sensible way to proceed is to leave it as is for now and eventually redo the whole "inventory from map" feature, taking into account things that have been proposed here like crafting time penalties for distance / piling etc.

@Sheco
Copy link
Contributor

Sheco commented Jan 4, 2014

@v-tl Those scenarios would be acceptable given the proper time penalties.

If you choose to use a distant tool or component, it will be inefficient.

But I agree the "inventory from map" is not ready, the most obvious defect would be the dialog to pick a component when there are multiple matches, for example:

Use which
component:
1 scrap metal (nearby)
2) aluminum can (nearby)
3) plastic bottle

It's not clear where those components are.

So I propose changing this dialog to a "Look all items around the player" sidebar, filtered by the matching components/tools, it's up to the player to select the appropriate items.

@dwarfkoala
Copy link

Looks like the mess died down. Whatever turtlicious is doing seems to be dead.

I was initially worried about Kevin's judgement, but when I actually took the time to figure out what his complaints with it were, I really think he had a pretty solid reason for not merging #4904:

"I know you put a lot of time into this, but the fact remains that it simply
doesn't make sense to manipulate "crafting distance" based on furniture
type. Your argument that it makes things "easier to find" doesn't apply,
because it doesn't make it easier/faster, it makes it possible.
Similarly, the argument that "you can't spot things in a pile on the
ground" doesn't apply, because this also applies to single items on the
ground or on the shelf, which would have a negligible impact.
If the limited crafting distance is keeping people from making sensible
bases, we can simply increase crafting distance, after all it's a
completely arbitrary limit, at which point the rationale for the change
becomes moot."

Perhaps he could have explained it better, but the logic is sound.

@kevingranade
Copy link
Member Author

There's certainly room for improvement in selecting which items are used for crafting, but I don't see why that should block this change, which is simply to increase the range of crafting so that people can build and use workshops instead of having to shuttle around parts or make unreasonably large piles if items simply to be able to have them all in range. This is a simple change that tackles that specific issue, we can come back later and try and bundle up location and stacking information with the crafting inventory so that we can do the time-based adjustments that everyone agrees are what we want eventually. The scenarios that are problematic are 1. avoidable and 2. will occur already because people typically pile all their stuff up so that they can craft whatever they like without having to move things around. It simply changes "don't put items you don't want to use for crafting on your crafting piles" to "don't put items you don't want to use for crafting in your crafting room"

@Sheco
Copy link
Contributor

Sheco commented Jan 6, 2014

I support this PR.

But I think the qualities system needs improvement.

I would love to see items with more qualities, for example knives that could be used as screwdrivers, maces that could be used as hammers, and so on.

The recipe would list "requires an item with hammering quality" instead of listing all the hammer items.

@Turtlicious
Copy link
Contributor

@dwarfkoala Goons are the worst at organizing.

@Rivet-the-Zombie
Copy link
Member

So now that the major hostility has abated, am I going to get hate mail if I merge this?

@SilearFlare
Copy link

Nope. Go for it.

@Rivet-the-Zombie Rivet-the-Zombie merged commit d8f5bbd into CleverRaven:master Jan 13, 2014
@kevingranade kevingranade deleted the increase-crafting-distance branch April 26, 2014 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet