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

Map item processing refactoring #39942

Merged
merged 11 commits into from
Apr 26, 2020
Merged

Conversation

Hirmuolio
Copy link
Contributor

@Hirmuolio Hirmuolio commented Apr 26, 2020

Summary

SUMMARY: Infrastructure "Map item processing refactoring"

Purpose of change

The chain of functions for processing items on map is weird and hard to follow.
There are also many things that are in there but do not do anything.

Describe the solution

  • The processing functions pases along processor pointer to a function. The function is always the same process_map_items so this was pointless. Now they just call the function.
  • There is a string signal passed around. It is not used anywhere for anything. Removed signal.
  • z position is passed along in vehicle item processing but not used for anything. Removed gridz.
  • Boolean in process_items is always true and was only used in an or statement where it did nothing. Removed the boolean.
  • The only thing that process_active_items did was set the above unneeded boolean to true and then call process_items. Removed process_active_items and instead just call process_items. process_items was made public so it can be called.
  • process_item always passes along a boolean false. Just have false in the final palce.
  • process_map_items does nothing but call process_item. Replace process_item with process_map_items.

Describe alternatives you've considered

Let it stay as unreadable mess of functions calling other functions calling other functions without clearly stating what function is called while passing variables that do nothing.

Testing

Tests pass, items still get processed in inventory, on ground and in vehicles.

Additional context

There should be zero visible changes in gameplay.

@ifreund ifreund added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Apr 26, 2020
@KorGgenT
Copy link
Member

probably conflicts with #39406

@Hirmuolio
Copy link
Contributor Author

You haven't made any edits in these areas at least yet.

Copy link
Contributor

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for cleaning this up! I'll wait on the rest of the CI to merge.

@KorGgenT it looks like you've barely touched map.cpp in #39406, I don't think it'll cause conflicts.

@KorGgenT
Copy link
Member

oh good, i was worried it would cause a merge conflict like the last one. i only skimmed it, i didn't try to review it.

@ifreund ifreund self-assigned this Apr 26, 2020
@ifreund ifreund merged commit ea7d0d3 into CleverRaven:master Apr 26, 2020
Drewscriver pushed a commit to Drewscriver/Cataclysm-DDA that referenced this pull request Apr 30, 2020
* remove processor

* remove map_process_func

* remove signal and gridz

* unused boolean in process_items

* removed process_items

* removed process_active_items instead of process_items

* made process_items public

* astyle

* activate is always false here

* remove process_item

* astyle
@Hirmuolio Hirmuolio deleted the processing branch May 5, 2020 12:33
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: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants