-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Implement Flood Fill Crafting Search #30444
Merged
kevingranade
merged 3 commits into
CleverRaven:master
from
jmattspartacus:Craft-Flood-Fill-Search-2
May 21, 2019
Merged
Implement Flood Fill Crafting Search #30444
kevingranade
merged 3 commits into
CleverRaven:master
from
jmattspartacus:Craft-Flood-Fill-Search-2
May 21, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OrenAudeles
suggested changes
May 12, 2019
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.
Small issues that are preventing successful compile.
c9c6311
to
0989cac
Compare
Initial commit, recommit of PR30399's code because of git issues I was unable to overcome. Adds: map::reachable_flood_steps() map::check_reachables() Removed function qualification in header Co-Authored-By: OrenAudeles <orenaudeles@gmail.com> Remove function qualification in header Co-Authored-By: OrenAudeles <orenaudeles@gmail.com>
map member functions: remove map:: qualifier to fix compile error astyling astyle check was failing on gorgon build, should fix that. Remove unnecessary functions Removing functions left over from the original implementation to prevent some code bloat where possible.
0989cac
to
0d22446
Compare
Probably breaks Android build (see https://ci.narc.ro/job/Cataclysm-Android/8882/console):
|
Breaks OS X builds:
|
ifreund
added a commit
to ifreund/Cataclysm-DDA
that referenced
this pull request
May 21, 2019
in pr CleverRaven#30444 vector::insert was called with `tripoint_range::begin()` which returns a `point_generator`. This call is however invalid since the called overload of `vector::insert()` requires the parameter to be move assignable. `point_generator` is not implicitly copy assignable as it contains a reference as a field and is therefore also not move assignable. This fixes the issue by replacing the `insert()` call with a range based for loop.
Merged
ifreund
added a commit
to ifreund/Cataclysm-DDA
that referenced
this pull request
May 21, 2019
in pr CleverRaven#30444 vector::insert was called with `tripoint_range::begin()` which returns a `point_generator`. This call is however invalid since the called overload of `vector::insert()` requires the parameter to be move assignable. `point_generator` is not implicitly copy assignable as it contains a reference as a field and is therefore also not move assignable. This fixes the issue by replacing the `insert()` call with a range based for loop.
ifreund
added a commit
to ifreund/Cataclysm-DDA
that referenced
this pull request
May 21, 2019
Code using inventories created with `inventory::form_from_map()` expects the origin to be included, which is not always the case after CleverRaven#30444. This includes the crafting code, where the issues caused by this are most obvious.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
[C++]
Changes (can be) made in C++. Previously named `Code`
Code: Performance
Performance boosting code (CPU, memory, etc.)
Crafting / Construction / Recipes
Includes: Uncrafting / Disassembling
Fields / Furniture / Terrain / Traps
Objects that are part of the map or its features.
Inventory / AIM / Zones
Inventory, Advanced Inventory Management or Zones
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial commit, recommit of PR30399's code because of git issues I was unable to overcome at the time.
Adds:
map::reachable_flood_steps()
How to use
Activate the crafting menu, and see if the crafting menu includes all things within 6 tiles that are not blocked off by impassible tiles, or spaces without floors
Summary
Summary: Features "Crafting inventory now uses flood fill search"
Makes the search for items for the crafting inventory use a flood fill for the search rather than only looking for items that are within 6 tiles in a straight line.
Purpose of change
When crafting, it's nice to have things in organized containers, but with the current search, it makes it so everything either has to be on the floor, or directly in sight. With a flood fill, it's possible to organize crates or shelves in rows, and provided they are within range, the item will be added to the crafting inventory.
Describe the solution
This flood fills tiles, starting from the player's location across the entire reachable area that is passable, and then if the location is found, the items in them are added to the crafting inventory.
Describe alternatives you've considered
I've considered hooking into the pathfinding code, but I'm not sure it's the right choice for this, as I don't understand all of the code there.
Comment:
I was unable to remove the changes from PR30399 that included files completely unrelated to the PR, I tried rebasing without any luck, and ended up making the situation worse.