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

Compute can_holser once per save to speed up d: filter from ~20s to ~5s #78950

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/item.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5994,15 +5994,7 @@ void item::final_info( std::vector<iteminfo> &info, const iteminfo_query *parts,
}

// does the item fit in any holsters?
std::vector<const itype *> holsters = Item_factory::find( [this]( const itype & e ) {
if( !e.can_use( "holster" ) ) {
return false;
}
const holster_actor *ptr = dynamic_cast<const holster_actor *>
( e.get_use( "holster" )->get_actor_ptr() );
const item holster_item( &e );
return ptr->can_holster( holster_item, *this ) && !item_is_blacklisted( holster_item.typeId() );
} );
std::vector<const itype *> holsters = item_controller->find_holster_for( *type );

if( !holsters.empty() && parts->test( iteminfo_parts::DESCRIPTION_HOLSTERS ) ) {
insert_separation_line( info );
Expand Down
22 changes: 22 additions & 0 deletions src/item_factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4832,6 +4832,7 @@ void Item_factory::reset()
void Item_factory::clear()
{
m_template_groups.clear();
type_contained_in_holsters.clear();

iuse_function_list.clear();

Expand Down Expand Up @@ -5469,6 +5470,27 @@ std::vector<const itype *> Item_factory::find( const std::function<bool( const i
return res;
}

const std::vector<const itype *> &Item_factory::find_holster_for( const itype &it )
{
const itype_id iid = it.get_id();
if( type_contained_in_holsters.count( iid ) > 0 ) {
return type_contained_in_holsters.at( iid );
}
const item itm( &it );

type_contained_in_holsters[iid] = Item_factory::find( [&itm]( const itype & e ) {
if( !e.can_use( "holster" ) ) {
return false;
}
const holster_actor *ptr = dynamic_cast<const holster_actor *>
( e.get_use( "holster" )->get_actor_ptr() );
const item holster_item( &e );
return ptr->can_holster( holster_item, itm );
} );

return type_contained_in_holsters[iid];
}

std::list<itype_id> Item_factory::subtype_replacement( const itype_id &base ) const
{
std::list<itype_id> ret;
Expand Down
11 changes: 11 additions & 0 deletions src/item_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,14 @@ class Item_factory

/** Find all item templates (both static and runtime) matching UnaryPredicate function */
static std::vector<const itype *> find( const std::function<bool( const itype & )> &func );
/**
* Find all holsters for itype. Uses type_contained_in_holsters cache, so it's much faster
* than constructing the result on the fly.
*
* This internally constructs an empty item from the itype. It should not be used as replacement
* for item::can_holster (for non-empty items). Ignores blacklist too.
*/
const std::vector<const itype *> &find_holster_for( const itype & );

std::list<itype_id> subtype_replacement( const itype_id & ) const;

Expand All @@ -285,6 +293,9 @@ class Item_factory
std::unordered_map<itype_id, ammotype> migrated_ammo;
std::unordered_map<itype_id, itype_id> migrated_magazines;

// cache for holsters
std::unordered_map<itype_id, std::vector<const itype *>> type_contained_in_holsters;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::vector<std::reference_wrapper<const itype>> would probably be a better fit than a std::vector<const itype *>>.

Copy link
Contributor Author

@Brambor Brambor Jan 5, 2025

Choose a reason for hiding this comment

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

Actually, Item_factory::find returns std::vector<const itype *>. So I would reinterpret it here as a vector of references. (No big deal)

It seems that whenever working with vector of itype, we use pointers. It might be that the code is old.

Do you still think I should change it to refs?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that whenever working with vector of itype, we use pointers. It might be that the code is old.

I think it is just that the code is old, and everyone who has used the code since then has just kept it as is. There isn't any performance cost to converting a pointer to a reference, so I think they should be stored as references.


/** Checks that ammo is listed in ammunition_type::name().
* At least one instance of this ammo type should be defined.
* If any of checks fails, prints a message to the msg stream.
Expand Down
Loading