Skip to content

Conversation

@swagween
Copy link
Contributor

need help with line 106 in player.cpp

@swagween swagween changed the title Lana/particles Adjust particle visuals Mar 15, 2024
…tion emitters and player exhaust.

# Conflicts:
#	src/spaced/spaced/game/hud.cpp
# Conflicts:
#	src/spaced/spaced/scenes/game.cpp
@swagween swagween requested a review from karnkaul March 15, 2024 16:17
karnkaul
karnkaul previously approved these changes Mar 16, 2024
@karnkaul karnkaul dismissed their stale review March 16, 2024 02:59

Changes needed

karnkaul
karnkaul previously approved these changes Mar 16, 2024
#include <spaced/game/controller.hpp>
#include <spaced/game/health.hpp>
#include <spaced/game/weapon.hpp>
#include <spaced/services/styles.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Move this include to the cpp file.

StringMap<bave::Rgba> rgbas{};
StringMap<ui::ButtonStyle> buttons{};
StringMap<ui::ProgressBarStyle> progress_bars{};
StringMap<ui::ProgressBarStyle> player{};
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this member.


auto const& rgbas = get_services().get<Styles>().rgbas;
auto spawn = [this] {
auto spawn = [this, rgbas] {
Copy link
Member

Choose a reason for hiding this comment

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

Don't copy rgbas by value in the lambda, take it by reference instead; we know that the Styles reference will outlive m_enemy_spawner (because it's a service indirectly owned by Spaced) so it's safe. [this, &rgbas]

auto spawn = [this, rgbas] {
auto ret = std::make_unique<Creep>(get_services(), this);
ret->shape.tint = bave::yellow_v;
ret->shape.tint = random_in_range(-1.f, 1.f) < 0.f ? rgbas["orange"] : rgbas["milk"];
Copy link
Member

@karnkaul karnkaul Mar 16, 2024

Choose a reason for hiding this comment

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

Why not random_in_range(0, 1) == 0? Floating point randomness is more computationally expensive, and you just want a binary range (this or that), not a theoretically infinite range of floats.

#include <spaced/services/layout.hpp>
#include <spaced/services/resources.hpp>
#include <spaced/services/scene_switcher.hpp>
#include <spaced/services/styles.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Unused header?

#include <spaced/util.hpp>

namespace spaced {
using bave::Rgba;
Copy link
Member

Choose a reason for hiding this comment

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

Unused alias?

void Home::on_loaded() { create_ui(); }

void Home::create_ui() {

Copy link
Member

Choose a reason for hiding this comment

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

Stray new line.

@karnkaul karnkaul dismissed their stale review March 16, 2024 03:04

goddammit, pressed approve instead of request changes

Copy link
Member

@karnkaul karnkaul left a comment

Choose a reason for hiding this comment

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

Refer to comments from the previous (mistakenly approved and then dismissed) review for requested changes.

@swagween swagween requested a review from karnkaul March 16, 2024 10:40
@karnkaul karnkaul enabled auto-merge (squash) March 17, 2024 03:36
@karnkaul karnkaul merged commit dbaaf57 into main Mar 17, 2024
@karnkaul karnkaul deleted the lana/particles branch March 17, 2024 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants