Fix lag from copying players and make them noncopyable #26331

jbytheway · 2018-10-20T21:03:52Z

Summary

SUMMARY: Bugfixes "Fix lag due to copying player objects"

Purpose of change

Issue #26147 described a few situations where lag was caused by copying player objects. We shouldn't ever be copying player objects. Some cases were fixed in #26272. This deals with the rest, and fixes #26147.

Describe the solution

To ensure this problem is gone and cannot accidentally return, we make player noncopyable. That also requires that npc be noncopyable. Since it was easy, I also made Character noncopyable.

The remaining situations where such objects were being copied were:

The new character screen, in order to predict recipes after skill selection.
The wash clothes window, for no obvious reason.
The multidrop window, in order to predict weight and volume after dropping.
The NPC trade screen, in order to predict NPC weight and volume after trading.
Some tests.

To resolve all these required some changes:

Calculations of whether recipes were possible are moved into SkillLevelMap from player and Character. So the new character screen can just copy that.
Weight and volume functions get new, more flexible versions which can account for predicting weight or volume after the inventory has some change made to it.
The tests can no longer copy a model NPC; they must create one anew each time.

As a side-effect, the wash clothes window no longer displays weight and volume at all (it served no purpose).

Describe alternatives you've considered

The messiest one to get working was the multidrop window. The way it handles item stacks is fairly weird. I suspect there's an opportunity there for further refactoring, but that would be a bigger change than seems sensible for this bugfix.

Additional context

This lays the groundwork for an improvement to the wash clothes window; it should be easy now to put water and soap requirements in the area where weight and volume previously were, but I don't want to do that on this PR.

Move various things from player and Character to SkillLevelMap so that code doesn't need an entire player object to access this information.

To avoid copying players, we need to be able to query Character volume and weight under the assumption that the items carried have been altered in some way. This adds the functionality to do so.

inventory_multiselector required a player object to compute the volume and weight stats. This is not feasible without copying the player, which we don't want to do. Therefore, change the interface so that the virtual function is returning the stats themselves, rather than the player from which they should be extracted. This also removes the need for ugly mutable data members. Fixes CleverRaven#26147 (both drop and washing clothing lag should be gone, since we assume they arose from unnecessary player copies). This lays the foundation for a better clothes washing interface that lets you know how much soap and water is required, but that is not implemented yet.

Rather than working with a copy of the npc, we rather work with just a copy of their inventory.

Rather than copying the player, we simply copy their skills.

Copying players was the cause of late-game lag which was hard to notice in simple testing (CleverRaven#26147). By making them noncopyable we prevent accidental copies.

Old gcc versions don't like this initialization.

BevapDin · 2018-10-21T09:40:43Z

src/skill.cpp

+int SkillLevelMap::get_skill_level( const skill_id &ident, const item &context ) const
+{
+    auto id = context.is_null() ? ident : context.contextualize_skill( ident );
+    return get_skill_level( std::move( id ) );


The move here is useless, the other get_skill_level function takes its argument as reference anyway.

True. It's also harmless, and might help in the future if that function changes. But I can see that's unlikely, so happy to change if you wish.

On reflection, it has a performance impact on debug builds, so I'll get rid of it.

BevapDin · 2018-10-21T09:43:47Z

src/character.h

+                replace_inv( &r )
+            {}
+            const std::map<const item *, int> *without_items = nullptr;
+            const inventory *replace_inv = nullptr;


Any reasons that this are pointers? It could be references, or optional<T> if you want a "not used" value.

They can't be references because they might not be present. And optional values are expensive to copy. I experimented with various ways of writing the code, and this seemed the cleanest.

In that case, I recommend optional<std::reference_wrapper<T>>. It

indicates that the value is optional,

that (if it exists) is a reference (never null),

is fast to copy as reference_wrapper is basically just a pointer,

is easy to distinguish from a pointer (which may indicate ownership, or may require dynamic memory allocation, who knows).

BevapDin · 2018-10-21T09:44:18Z

src/character.h

+        };
+
+        units::mass weight_carried_with_tweaks( item_tweaks ) const;
+        units::volume volume_carried_with_tweaks( item_tweaks ) const;


Any reason these functions take by value and not by const reference?

No particular reason; would work fine either way.

Previous workaround broke things for clang, so add even more braces!

Replace with optional<reference_wrapper<T>>.

These objects are now more heavyweight, so it makes sense to pass by const reference rather than value.

Prior changes on this branch to the character creation process meant that the recipes reported during skill selection were unreliable and recipes learnt during skill selection would be retained in game. This is because calling player::knows_recipe updates the known recipe cache. Avoid doing that, and reproduce the previous behaviour for listed recipes.

jbytheway added 7 commits October 20, 2018 20:58

Don't copy NPCs in tests

d94b14f

Move skill logic to SkillLevelMap

4edd9c3

Move various things from player and Character to SkillLevelMap so that code doesn't need an entire player object to access this information.

More flexible volume and weight query functions

8854a31

To avoid copying players, we need to be able to query Character volume and weight under the assumption that the items carried have been altered in some way. This adds the functionality to do so.

Don't copy npc during npc trading

c18d442

Rather than working with a copy of the npc, we rather work with just a copy of their inventory.

Don't copy player during character creation

7e8ecb6

Rather than copying the player, we simply copy their skills.

Make Character, player, and npc noncopyable

cdc33ff

Copying players was the cause of late-game lag which was hard to notice in simple testing (CleverRaven#26147). By making them noncopyable we prevent accidental copies.

jbytheway changed the title ~~Noncopyable character~~ Fix lag from copying players and make them noncopyable Oct 20, 2018

ZhilkinSerg self-assigned this Oct 21, 2018

Work around older gcc warnings

6342972

Old gcc versions don't like this initialization.

BevapDin reviewed Oct 21, 2018

View reviewed changes

jbytheway added 6 commits October 21, 2018 11:08

Work around clang warning

1833b31

Previous workaround broke things for clang, so add even more braces!

Avoid raw pointers

08ae8a0

Replace with optional<reference_wrapper<T>>.

Pass item_tweaks by reference

58b0032

These objects are now more heavyweight, so it makes sense to pass by const reference rather than value.

Remove redundant std::move

4d09d16

Move meets_skill_requirements to SkillLevelMap

d5a6b1c

ZhilkinSerg merged commit a0eb7f4 into CleverRaven:master Oct 23, 2018

ZhilkinSerg removed their assignment Oct 23, 2018

jbytheway deleted the noncopyable_character branch October 24, 2018 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lag from copying players and make them noncopyable #26331

Fix lag from copying players and make them noncopyable #26331

jbytheway commented Oct 20, 2018 •

edited

BevapDin Oct 21, 2018

jbytheway Oct 21, 2018

jbytheway Oct 21, 2018

BevapDin Oct 21, 2018

jbytheway Oct 21, 2018

BevapDin Oct 21, 2018

BevapDin Oct 21, 2018

jbytheway Oct 21, 2018

Fix lag from copying players and make them noncopyable #26331

Fix lag from copying players and make them noncopyable #26331

Conversation

jbytheway commented Oct 20, 2018 • edited

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Additional context

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Oct 20, 2018 •

edited