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

noulolhaha · 2024-03-04T01:35:05Z

Summary

Interface "Instead of rendering the last-dropped item on the ground, it renders the last-dropped favorite items on the ground, or the biggest item if no such favorite item."

Purpose of change

Addresses #71637

Once, I lost my backpack in a lab and spent like an hour trying to find it because after dropping my backpack on a tile, I noticed I also had some broken glass in my pockets and decided to drop that too. Because the game rendered the glass instead of the backpack. it was a huge pain to find it.

In general, it makes much more sense for bigger items to display in favor of smaller ones, and for favorite items to display in favor of non-favorite ones, and will prevent stuff like bullet casings covering up actually important items.

Describe the solution

In the code that renders dropped items, I replaced the calls to submap::get_uppermost_item() with a call to a new function, submap::get_most_visible_item(), which iterates through the items on that tile and returns the first (and therefore last dropped) favorite item or, failing that, the biggest item.

Describe alternatives you've considered

Marking the item as favorite and enabling creation of notes when a favorite item is dropped helps, but it doesn't give its exact position as well, and it doesn't change the fact that it's really weird/potentially deceptive for the last dropped item to render, even when you drop something tiny on something big.

Testing

I walked around in a new game picking up and dropping various items in various orders, and strolling through the city. Worked as expected, and I did not notice any performance issues, even with thousands of items on the same tile.
I also ran all tests locally and saw no related issues.

Additional context

If this leads to performance issues with item rendering (given the O(n items on tile) time complexity), it would be pretty simple to just cache the most visible item on each tile, but I can't imagine this being an issue unless there are literally >1,000,000 items in view, in which case there are most likely already other performance issues.

github-actions · 2024-03-04T01:36:12Z

src/cata_tiles.cpp

@@ -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 ) );


[JSON & C++ formatters] _{reported by reviewdog 🐶}

Suggested change

( entry ) );

( entry ) );

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).

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

+             std::remove_if(
+                 v.obj.begin(),
+                 v.obj.end(),


[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(),

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

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


[JSON & C++ formatters] _{reported by reviewdog 🐶}

Suggested change

);

);

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

+          std::remove_if(
+              vs.begin(),
+              vs.end(),


[JSON & C++ formatters] _{reported by reviewdog 🐶}

Suggested change

std::remove_if(

vs.begin(),

vs.end(),

std::remove_if(

vs.begin(),

vs.end(),

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

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


[JSON & C++ formatters] _{reported by reviewdog 🐶}

Suggested change

),

),

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

@@ -3579,7 +3578,7 @@
            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;


[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;

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

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


[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 ) );

github-actions · 2024-03-04T01:36:13Z

src/cata_tiles.cpp

@@ -4818,7 +4819,7 @@
    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;


[JSON & C++ formatters] _{reported by reviewdog 🐶}

Suggested change

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

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

github-actions · 2024-03-04T01:36:14Z

src/cata_tiles.cpp

@@ -5238,7 +5239,7 @@

    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(),


[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(),

github-actions · 2024-03-04T01:36:14Z

src/submap.h

+        *
+        * Assumes there is at least one item.
+        * Used to decide which item to render.
+        * TODO: might be slow enough to justify storing the most visible item for each tile? 


[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?

Brambor · 2024-03-04T01:56:36Z

src/submap.h

+        *
+        * Assumes there is at least one item.
+        * Used to decide which item to render.
+        * TODO: might be slow enough to justify storing the most visible item for each tile? 


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.

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

Brambor

I am not sure if this is viable, but if it is, the merger will want these to be fixed.

Brambor · 2024-03-04T02:03:51Z

src/submap.h

+            units::volume biggest_volume = 0_ml;
+
+            // uppermost item is at end of the list, so iterate backwards so uppermost item is preferred
+            auto items = get_items();


Avoid using auto

Brambor · 2024-03-04T02:04:02Z

src/submap.h

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


Avoid using auto

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.

Brambor · 2024-03-04T02:07:10Z

src/submap.h

@@ -418,6 +418,35 @@ class maptile_impl
            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.


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).

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

Brambor · 2024-03-04T02:08:44Z

src/submap.h

+        * Used to decide which item to render.
+        * TODO: might be slow enough to justify storing the most visible item for each tile? 
+        */
+        const item &get_most_visible_item() const {


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.

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.

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.

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

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

Brambor · 2024-03-04T02:16:19Z

I like the idea of showing a favourite item if it can be done fast. I believe this is not fast. But you can prove me wrong by testing it.

I don't care for showing the largest item.

SurFlurer · 2024-03-04T03:02:36Z

Would you like to test its performance impact?

SurFlurer · 2024-03-04T09:00:57Z

I got an error when playing with your changes.

Steps to reproduce:

Run CDDA with msvc debugger attached to it.
Load a world, wield something then start to craft something.
When the game asks you what to do with the in-progress item, select "Put it down and start working".
The error pops up.

Specifically, itm at

Cataclysm-DDA/src/cata_tiles.cpp

Line 3720 in faf0ed7

if( itm.has_itype_variant() ) {

is null.

The same happens when I try to debug spawn 10000 plastic chunks.

SurFlurer · 2024-03-04T09:42:06Z

Once, I lost my backpack in a lab and spent like an hour trying to find it because after dropping my backpack on a tile, I noticed I also had some broken glass in my pockets and decided to drop that too. Because the game rendered the glass instead of the backpack. it was a huge pain to find it.

In case you are not aware, you can press "V" to list all items around the player.

And it's quite easy to locate whatever you're looking for, especially if you add the item to your favorite. Either scroll down the list or press "/" to search to filter the list.

noulolhaha · 2024-03-05T23:52:28Z

I did not mean to push just now, I am still making changes. Now I know what that button does.

noulolhaha · 2024-03-07T02:40:36Z

Alright, I fixed those crashes. After doing some profiling, I've found that it does make the game run slightly slower, which isn't great, so I'm making submaps store the most visible item on each tile so it only needs to be recalculated when an item is added (just compare new item to current most visible, O(1)) or when the most visible item is removed (O(N), but this isn't going to happen very often and worst case it's iterating through 4096 items in contiguous memory).

noulolhaha · 2024-03-07T03:15:24Z

On further examination, that might not actually be possible since apparently there's a method used throughout the code that just gives people a read-write reference to the items, meaning there's no way to know when an item is added/removed. I'll see if I can find a clever workaround for performance, though.

If I can't, I'll just make it ignore volume when deciding what to render, since calling base_volume() was the main slowdown in get_most_visible_item(), and make it just prioritize favorite items.

noulolhaha · 2024-03-08T03:58:31Z

After more performance testing with ~300,000 dropped items on the ground, I found that this just won't work performance wise, even if it ignores volume and just tries to prioritize favorite items. Given that it's just a small QOL feature it's definitely not worth the performance hit, so I'm gonna close this. Sad.

Inglonias · 2024-03-08T03:59:54Z

Better luck next time. Sorry things didn't work out.

yay it works

faf0ed7

github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. new contributor labels Mar 4, 2024

github-actions bot reviewed Mar 4, 2024

View reviewed changes

Brambor reviewed Mar 4, 2024

View reviewed changes

Brambor suggested changes Mar 4, 2024

View reviewed changes

github-actions bot added json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 4, 2024

Brambor mentioned this pull request Mar 4, 2024

docs on compiling: friendlier to first-time contributors #72166

Merged

redpenguiney added 2 commits March 5, 2024 15:16

address some complaints in pr

7b3ae8f

address some complaints in the pr

f0d5733

noulolhaha closed this Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

noulolhaha commented Mar 4, 2024

github-actions bot Mar 4, 2024

Brambor Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

github-actions bot Mar 4, 2024

Brambor Mar 4, 2024 •

edited

Loading

noulolhaha Mar 4, 2024

Brambor left a comment

Brambor Mar 4, 2024

Brambor Mar 4, 2024

mqrause Mar 4, 2024

Brambor Mar 4, 2024

noulolhaha Mar 4, 2024

Brambor Mar 4, 2024 •

edited

Loading

noulolhaha Mar 4, 2024

Brambor Mar 4, 2024

noulolhaha Mar 4, 2024

Brambor Mar 4, 2024

Brambor commented Mar 4, 2024

SurFlurer commented Mar 4, 2024

SurFlurer commented Mar 4, 2024 •

edited

Loading

SurFlurer commented Mar 4, 2024 •

edited

Loading

noulolhaha commented Mar 5, 2024

noulolhaha commented Mar 7, 2024

noulolhaha commented Mar 7, 2024

noulolhaha commented Mar 8, 2024

Inglonias commented Mar 8, 2024

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

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

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

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

	* 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?

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

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

Conversation

noulolhaha commented Mar 4, 2024

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

github-actions bot Mar 4, 2024

Choose a reason for hiding this comment

Brambor Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brambor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brambor Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brambor commented Mar 4, 2024

SurFlurer commented Mar 4, 2024

SurFlurer commented Mar 4, 2024 • edited Loading

SurFlurer commented Mar 4, 2024 • edited Loading

noulolhaha commented Mar 5, 2024

noulolhaha commented Mar 7, 2024

noulolhaha commented Mar 7, 2024

noulolhaha commented Mar 8, 2024

Inglonias commented Mar 8, 2024

Brambor Mar 4, 2024 •

edited

Loading

Brambor Mar 4, 2024 •

edited

Loading

SurFlurer commented Mar 4, 2024 •

edited

Loading

SurFlurer commented Mar 4, 2024 •

edited

Loading