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

Stacks of items on the ground render either the last-dropped favorite item or the largest item. #72153

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
57 changes: 29 additions & 28 deletions src/cata_tiles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ void tileset_cache::loader::create_textures_from_tile_atlas( const SDL_Surface_P
for( tiles_pixel_color_entry &entry : tile_values_data ) {
std::vector<texture> *tile_values = std::get<0>( entry );
color_pixel_function_pointer color_pixel_function = get_color_pixel_function( std::get<1>
( entry ) );
( entry ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
( entry ) );
( entry ) );

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: You can accept all the bot is suggesting here in a single commit, (then squish that commit into yours locally, if you want, then force push).

if( !color_pixel_function ) {
// TODO: Move it inside apply_color_filter.
copy_surface_to_texture( tile_atlas, offset, *tile_values );
Expand Down Expand Up @@ -934,26 +934,26 @@ void tileset_cache::loader::process_variations_after_loading(
for( auto &v : vs ) {
// in a given variation, erase any invalid sprite ids
v.obj.erase(
std::remove_if(
v.obj.begin(),
v.obj.end(),
std::remove_if(
v.obj.begin(),
v.obj.end(),
Comment on lines +937 to +939
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
std::remove_if(
v.obj.begin(),
v.obj.end(),
std::remove_if(
v.obj.begin(),
v.obj.end(),

[&]( int id ) {
return id >= offset || id < 0;
} ),
v.obj.end()
);
);
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
);
);

}
// erase any variations with no valid sprite ids left
vs.erase(
std::remove_if(
vs.begin(),
vs.end(),
std::remove_if(
vs.begin(),
vs.end(),
Comment on lines +948 to +950
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
std::remove_if(
vs.begin(),
vs.end(),
std::remove_if(
vs.begin(),
vs.end(),

[&]( const weighted_object<int, std::vector<int>> &o ) {
return o.obj.empty();
}
),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
),
),

vs.end()
);
);
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
);
);

// populate the bookkeeping table used for selecting sprite variations
vs.precalc();
}
Expand Down Expand Up @@ -1365,9 +1365,9 @@ void cata_tiles::draw( const point &dest, const tripoint &center, int width, int
here.prev_o = o;

you.prepare_map_memory_region(
here.getglobal( tripoint( min_mm_reg, center.z ) ),
here.getglobal( tripoint( max_mm_reg, center.z ) )
);
here.getglobal( tripoint( min_mm_reg, center.z ) ),
here.getglobal( tripoint( max_mm_reg, center.z ) )
);
Comment on lines +1368 to +1370
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
here.getglobal( tripoint( min_mm_reg, center.z ) ),
here.getglobal( tripoint( max_mm_reg, center.z ) )
);
here.getglobal( tripoint( min_mm_reg, center.z ) ),
here.getglobal( tripoint( max_mm_reg, center.z ) )
);


//set up a default tile for the edges outside the render area
visibility_type offscreen_type = visibility_type::HIDDEN;
Expand Down Expand Up @@ -1601,8 +1601,8 @@ void cata_tiles::draw( const point &dest, const tripoint &center, int width, int

// string overlay
here.overlay_strings_cache.emplace(
tile_pos + quarter_tile,
formatted_text( text, catacurses::black, direction::NORTH ) );
tile_pos + quarter_tile,
formatted_text( text, catacurses::black, direction::NORTH ) );
Comment on lines +1604 to +1605
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
tile_pos + quarter_tile,
formatted_text( text, catacurses::black, direction::NORTH ) );
tile_pos + quarter_tile,
formatted_text( text, catacurses::black, direction::NORTH ) );

};

if( g->display_overlay_state( ACTION_DISPLAY_LIGHTING ) ) {
Expand Down Expand Up @@ -1709,7 +1709,7 @@ void cata_tiles::draw( const point &dest, const tripoint &center, int width, int
int cur_zlevel = draw_min_z;
while( cur_zlevel <= center.z ) {
const half_open_rectangle<point> &cur_any_tile_range = is_isometric()
? z_any_tile_range[center.z - cur_zlevel] : top_any_tile_range;
? z_any_tile_range[center.z - cur_zlevel] : top_any_tile_range;
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
? z_any_tile_range[center.z - cur_zlevel] : top_any_tile_range;
? z_any_tile_range[center.z - cur_zlevel] : top_any_tile_range;

// For each row
for( int row = cur_any_tile_range.p_min.y; row < cur_any_tile_range.p_max.y; row ++ ) {
// Set base height for each tile
Expand Down Expand Up @@ -3276,7 +3276,7 @@ bool cata_tiles::draw_furniture( const tripoint &p, const lit_level ll, int &hei
const auto furn = [&]( const tripoint & q, const bool invis ) -> furn_id {
const auto it = furniture_override.find( q );
return it != furniture_override.end() ? it->second :
( !overridden || !invis ) ? here.furn( q ) : f_null;
( !overridden || !invis ) ? here.furn( q ) : f_null;
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
( !overridden || !invis ) ? here.furn( q ) : f_null;
( !overridden || !invis ) ? here.furn( q ) : f_null;

};
const std::array<int, 4> neighborhood = {
static_cast<int>( furn( p + point_south, invisible[1] ) ),
Expand Down Expand Up @@ -3370,7 +3370,7 @@ bool cata_tiles::draw_trap( const tripoint &p, const lit_level ll, int &height_3
const auto tr_at = [&]( const tripoint & q, const bool invis ) -> trap_id {
const auto it = trap_override.find( q );
return it != trap_override.end() ? it->second :
( !overridden || !invis ) ? here.tr_at( q ).loadid : tr_null;
( !overridden || !invis ) ? here.tr_at( q ).loadid : tr_null;
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
( !overridden || !invis ) ? here.tr_at( q ).loadid : tr_null;
( !overridden || !invis ) ? here.tr_at( q ).loadid : tr_null;

};
const std::array<int, 4> neighborhood = {
static_cast<int>( tr_at( p + point_south, invisible[1] ) ),
Expand Down Expand Up @@ -3473,8 +3473,7 @@ bool cata_tiles::draw_field_or_item( const tripoint &p, const lit_level ll, int
auto has_field = [&]( field_type_id fld, const tripoint & q, const bool invis ) -> field_type_id {
// go through the fields and see if they are equal
field_type_id found = fd_null;
for( std::pair<const field_type_id, field_entry> &this_fld : here.field_at( q ) )
{
for( std::pair<const field_type_id, field_entry> &this_fld : here.field_at( q ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
for( std::pair<const field_type_id, field_entry> &this_fld : here.field_at( q ) ) {
for( std::pair<const field_type_id, field_entry> &this_fld : here.field_at( q ) )
{

if( this_fld.first == fld ) {
found = fld;
}
Expand Down Expand Up @@ -3579,7 +3578,7 @@ bool cata_tiles::draw_field_or_item( const tripoint &p, const lit_level ll, int
auto field_at = [&]( const tripoint & q, const bool invis ) -> field_type_id {
const auto it = field_override.find( q );
return it != field_override.end() ? it->second :
( !fld_overridden || !invis ) ? here.field_at( q ).displayed_field_type() : fd_null;
( !fld_overridden || !invis ) ? here.field_at( q ).displayed_field_type() : fd_null;
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
( !fld_overridden || !invis ) ? here.field_at( q ).displayed_field_type() : fd_null;
( !fld_overridden || !invis ) ? here.field_at( q ).displayed_field_type() : fd_null;

};
// for rotation information
const std::array<int, 4> neighborhood = {
Expand Down Expand Up @@ -3648,7 +3647,7 @@ bool cata_tiles::draw_field_or_item( const tripoint &p, const lit_level ll, int
0, layer_lit, layer_nv, height_3d, 0, variant, layer_var.offset );

// if the top item is already being layered don't draw it later
if( i.typeId() == tile.get_uppermost_item().typeId() ) {
if( i.typeId() == tile.get_most_visible_item().typeId() ) {
drawtop = false;
}

Expand Down Expand Up @@ -3693,7 +3692,7 @@ bool cata_tiles::draw_field_or_item( const tripoint &p, const lit_level ll, int
0, layer_lit, layer_nv, height_3d, 0, variant, layer_var.offset );

// if the top item is already being layered don't draw it later
if( i.typeId() == tile.get_uppermost_item().typeId() ) {
if( i.typeId() == tile.get_most_visible_item().typeId() ) {
drawtop = false;
}

Expand All @@ -3715,7 +3714,9 @@ bool cata_tiles::draw_field_or_item( const tripoint &p, const lit_level ll, int
hilite = std::get<2>( it_override->second );
it_type = item::find_type( it_id );
} else if( !invisible[0] && here.sees_some_items( p, get_player_character() ) ) {
const item &itm = tile.get_uppermost_item();
// instead of just drawing the uppermost item, we draw the uppermost favorite item, and if there is none, we draw the item with the greatest volume.
const item &itm = tile.get_most_visible_item();

if( itm.has_itype_variant() ) {
variant = itm.itype_variant().id;
}
Expand Down Expand Up @@ -4717,8 +4718,8 @@ void cata_tiles::draw_sct_frame( std::multimap<point, formatted_text> &overlay_s
full_text_length / 2;

overlay_strings.emplace(
player_to_screen( iD + point( direction_offset, 0 ) ),
formatted_text( sText, FG, direction ) );
player_to_screen( iD + point( direction_offset, 0 ) ),
formatted_text( sText, FG, direction ) );
Comment on lines +4721 to +4722
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
player_to_screen( iD + point( direction_offset, 0 ) ),
formatted_text( sText, FG, direction ) );
player_to_screen( iD + point( direction_offset, 0 ) ),
formatted_text( sText, FG, direction ) );

} else {
for( char &it : sText ) {
const std::string generic_id = get_ascii_tile_id( it, FG, -1 );
Expand Down Expand Up @@ -4818,7 +4819,7 @@ void cata_tiles::get_terrain_orientation( const tripoint &p, int &rota, int &sub
const auto ter = [&]( const tripoint & q, const bool invis ) -> ter_id {
const auto override = ter_override.find( q );
return override != ter_override.end() ? override->second :
( !overridden || !invis ) ? here.ter( q ) : t_null;
( !overridden || !invis ) ? here.ter( q ) : t_null;
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
( !overridden || !invis ) ? here.ter( q ) : t_null;
( !overridden || !invis ) ? here.ter( q ) : t_null;

};

// get terrain at x,y
Expand Down Expand Up @@ -5238,7 +5239,7 @@ void cata_tiles::do_tile_loading_report()

std::vector<map_extra_id> map_extra_ids = MapExtras::get_all_function_names();
map_extra_ids.erase(
std::remove_if( map_extra_ids.begin(), map_extra_ids.end(),
std::remove_if( map_extra_ids.begin(), map_extra_ids.end(),
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
std::remove_if( map_extra_ids.begin(), map_extra_ids.end(),
std::remove_if( map_extra_ids.begin(), map_extra_ids.end(),

[]( const map_extra_id & id ) {
return !id->autonote;
} ), map_extra_ids.end() );
Expand Down
29 changes: 29 additions & 0 deletions src/submap.h
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,35 @@
return *std::prev( sm->get_items( pos() ).cend() );
}

/** Returns the uppermost favorite item if there is a favorite item. Otherwise, returns the item with the greatest volume.
*
* Assumes there is at least one item.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check it in the code instead of silently returning nullptr. Throw some cata error (not sure how from the top of my head).

Copy link
Author

Choose a reason for hiding this comment

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

I put that there because get_uppermost_item() made that assumption, so I figured I could too. I'll change it though.

* Used to decide which item to render.
* TODO: might be slow enough to justify storing the most visible item for each tile?
Copy link
Contributor

Choose a reason for hiding this comment

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

[JSON & C++ formatters] reported by reviewdog 🐶

Suggested change
* TODO: might be slow enough to justify storing the most visible item for each tile?
* TODO: might be slow enough to justify storing the most visible item for each tile?

Copy link
Contributor

@Brambor Brambor Mar 4, 2024

Choose a reason for hiding this comment

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

You can (and should!) check this yourself.
Spawn a ton of items and see if the game is still playable before and after your changes.

Copy link
Author

Choose a reason for hiding this comment

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

I actually did test it ingame, spawned a bunch of items, I mentioned that in the PR but didn't update the comment in the code, mb

*/
const item &get_most_visible_item() const {
Copy link
Contributor

@Brambor Brambor Mar 4, 2024

Choose a reason for hiding this comment

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

The implementation of this size should be in the .cpp file, not the .h file.

Btw this will improve the compilation time (when changing only the implementation) enormously, as you only recompile one file.

Copy link
Author

Choose a reason for hiding this comment

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

I realized that - I was again just basing it off what get_uppermost_item() did, and assumed that there was some reason that other functions in the same file were implemented in the header. I'll fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's based on simplicity of the function. One liners can be in .h files. But anything in .h means drastically more recompile time on change.

Copy link
Author

Choose a reason for hiding this comment

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

oh wait, actually it's all in a header because it's a method of a template class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you might be right. I don't know enough about template stuff splitting, Feel free to mark this as resolved.


const item *biggest_item = nullptr;
units::volume biggest_volume = 0_ml;

Check failure on line 430 in src/submap.h

View workflow job for this annotation

GitHub Actions / build (src)

integer literal has suffix 'l', which is not uppercase [cert-dcl16-c,readability-uppercase-literal-suffix,-warnings-as-errors]

// uppermost item is at end of the list, so iterate backwards so uppermost item is preferred
auto items = get_items();
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using auto

// unsigned int i = get_items().size();
for( auto it = items.rbegin(); it != items.rend(); it++ ) {
const auto &item = *it;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using auto

Copy link
Contributor

Choose a reason for hiding this comment

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

It's generally acceptable for more complex types such as iterators, though. So only the one inside the loop needs to be changed, in case that wasn't clear.


if( item.is_favorite ) {
return item;
}

if( item.base_volume() > biggest_volume ) {
biggest_item = &item;
biggest_volume = item.base_volume();
}
}
return *biggest_item;
}

// Gets all items
const cata::colony<item> &get_items() const {
return sm->get_items( pos() );
Expand Down
Loading