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

Remove optional maxitems from map::add_item{,_at} #10700

Merged
merged 1 commit into from Jan 1, 2015

Conversation

Projects
None yet
3 participants
@narc0tiq
Copy link
Contributor

commented Jan 1, 2015

It turns out the parameter wasn't really being useful anymore. It's an artifact from when the add_item_at code would actually perform item overflow itself -- later, that got moved up into map::add_item_or_charges, but the parameter was kept... with a simple "return;" in place of the overflow code.

Ordinarily, this wouldn't be a problem -- the only way to call into map::add_item was from map itself, namely from map::add_item_or_charges. Which checked for overflow and didn't even call into add_item if it would enter that code path.

However, recently, map::add_item got made public (#10293) so it could be used to bypass terrain flag checks (specifically, the NOITEM checks that would cause items to spill over into adjacent tiles. Remember gas pumps with their fuel next to them? Yeah, that bug). When the new calls to add_item were added, they omitted the optional parameter, so the code defaulted to... 64.

This was still not a problem! Most functions using this were doing it in mapgen, where the aforementioned fuel was placed onto the aforementioned gas pump, or similar places -- there would only ever be one (or, very rarely, two) items in the given tile. The evil bug would still not be activated until...

...active item processing needed a faster way to insert items back into the map (#10599). So it used map::add_item_at, because that's nice and quick. It makes perfect sense, too: there's no need to check most potential issues with dropping items into the given tile because we got the items from that tile -- ergo, they are supposed to be there.

BUT:

  • because the default maxitems was 64,
  • whenever a tile had more than 64 items in it,
  • any active items in that tile would get pulled out for processing...
  • ...and attempt to insert back in, which would silently fail, destroying the item.

Edit: Oh, I missed a reference: http://smf.cataclysmdda.com/index.php?topic=9022.0

Remove optional maxitems from map::add_item{,_at}
It turns out the parameter wasn't really being useful anymore. It's an
artifact from when the `add_item_at` code would actually perform item
overflow itself -- later, that got moved up into map
`add_item_or_charges`, but the parameter was kept... with a simple
"return;" in place of the overflow code.
@narc0tiq

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2015

Also, it occurs to me I should've named the branch "add-items-ate-my-chunks-of-meat". Because it did.

@KA101 KA101 self-assigned this Jan 1, 2015

@KA101 KA101 merged commit 54e5e52 into CleverRaven:master Jan 1, 2015

1 check passed

default This has been rescheduled for testing as the 'master' branch has been updated.

@narc0tiq narc0tiq deleted the narc0tiq:add-items-ate-my-cookies branch Jan 1, 2015

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jan 1, 2015

Oh crap, thanks a ton for fixing this. I blew about an hour trying to
reproduce this yesterday.

@narc0tiq

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2015

Yeah, it took a while just to find a good reproduction, but then afterwards
everything came together.

On Thu, Jan 1, 2015 at 8:27 PM, Kevin Granade notifications@github.com
wrote:

Oh crap, thanks a ton for fixing this. I blew about an hour trying to
reproduce this yesterday.


Reply to this email directly or view it on GitHub
#10700 (comment)
.

@narc0tiq narc0tiq referenced this pull request Jan 2, 2015

Closed

Disappearing items. #10610

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.