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

Avoid unnecessary dynamic_casts, LayerObject base class, remove CollisionListener #3110

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

Vankata453
Copy link
Member

@Vankata453 Vankata453 commented Nov 22, 2024

Unnecessary dynamic_casts have been replaced with static_casts, or the code requiring them has been improved.

Add LayerObject class

LayerObject is a base class for all GameObjects which are shown under the EditorLayersWidget. Used to prevent multiple casts every single frame for every single layer icon when determining z-pos.

Closes #701.

Remove CollisionListener class

The changes done by commit fa82f91 have been reverted. The reason is that CollisionListener was not used as a base anywhere beside MovingObject, and made it so CollisionObject::collide()/collision() had to use dynamic_cast to side-cast the other object's listener to a GameObject.

CollisionObject now contains the MovingObject& m_parent member, as opposed to CollisionListener& m_listener.

Additionally, the now base MovingObject::collision... functions now accept MovingObject& in arguments, instead of GameObject&. This saves on a few other casts along the way.

Unnecessary `dynamic_cast`s have been replaced with `static_cast`s, or the code requiring them has been improved.

`LayerObject` is a base class for all `GameObject`s which are shown under the `EditorLayersWidget`. Used to prevent multiple casts every single frame for every single layer icon when determining z-pos.
Reverts changes done by commit fa82f91. The reason is that `CollisionListener` was not used as a base anywhere beside `MovingObject`, and made it so `CollisionObject::collide()`/`collision()` had to use `dynamic_cast` to side-cast the `other` object's listener to a `GameObject`.

`CollisionObject` now contains the `MovingObject& m_parent` member, as opposed to `CollisionListener& m_listener`.

Additionally, the now base `MovingObject::collision...` functions now accept `MovingObject&` in arguments, instead of `GameObject&`. This saves on a few other `dynamic_cast`s along the way.
@Vankata453 Vankata453 added involves:performance category:code status:needs-review Work needs to be reviewed by other people labels Nov 22, 2024
@tobbi
Copy link
Member

tobbi commented Nov 23, 2024

I'm gonna test this in a few.

@tobbi
Copy link
Member

tobbi commented Nov 23, 2024

This also fixes #701

@tobbi
Copy link
Member

tobbi commented Nov 23, 2024

In file included from /Volumes/Development/workspace/supertux/src/badguy/scrystallo.cpp:21:
/Volumes/Development/workspace/supertux/src/object/player.hpp:198:7: warning: 'get_coins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  int get_coins() const;
      ^
/Volumes/Development/workspace/supertux/src/supertux/game_object.hpp:167:15: note: overridden virtual function is here
  virtual int get_coins() const { return 0; }
              ^
In file included from /Volumes/Development/workspace/supertux/src/badguy/short_fuse.cpp:23:
/Volumes/Development/workspace/supertux/src/object/player.hpp:198:7: warning: 'get_coins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  int get_coins() const;
      ^
/Volumes/Development/workspace/supertux/src/supertux/game_object.hpp:167:15: note: overridden virtual function is here
  virtual int get_coins() const { return 0; }
              ^
In file included from /Volumes/Development/workspace/supertux/src/badguy/root_sapling.cpp:23:
/Volumes/Development/workspace/supertux/src/object/player.hpp:198:7: warning: 'get_coins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  int get_coins() const;
      ^
/Volumes/Development/workspace/supertux/src/supertux/game_object.hpp:167:15: note: overridden virtual function is here
  virtual int get_coins() const { return 0; }
              ^
In file included from /Volumes/Development/workspace/supertux/src/badguy/skydive.cpp:21:
/Volumes/Development/workspace/supertux/src/object/player.hpp:198:7: warning: 'get_coins' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  int get_coins() const;
      ^
/Volumes/Development/workspace/supertux/src/supertux/game_object.hpp:167:15: note: overridden virtual function is here
  virtual int get_coins() const { return 0; }
              ^

@tobbi
Copy link
Member

tobbi commented Nov 23, 2024

I tested it, seems fine. I couldn't notice much of a benefit, only thing I might have noticed is bonus block hitting seems to be way less laggy (there used to be a slight lag before).

@swagtoy
Copy link

swagtoy commented Nov 25, 2024

Dynamic cast used in the collision code??????????????????? Good lord. This PR might bring about a substantial performance boost O_O

Best it gets merge asap to prevent any future blockers in other PRs?

src/editor/layer_object.cpp Outdated Show resolved Hide resolved
class LayerObject : public GameObject
{
public:
LayerObject(const std::string& name = "");
Copy link

Choose a reason for hiding this comment

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

std::move me. Compiler probably wouldn't care anyway.

src/object/bicycle_platform.cpp Show resolved Hide resolved
src/object/bonus_block.cpp Outdated Show resolved Hide resolved
src/object/player.cpp Show resolved Hide resolved
@swagtoy
Copy link

swagtoy commented Nov 25, 2024

Nothing else to say, just decide on std::move. lgmt :)

@Vankata453 Vankata453 merged commit 725a20a into master Nov 26, 2024
33 checks passed
@Vankata453 Vankata453 deleted the less-dynamic-cast branch November 26, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code involves:performance status:needs-review Work needs to be reviewed by other people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define Layer class for things that act as layers in the editor
4 participants