Skip to content

Adjust particle visuals #23

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

Merged
merged 6 commits into from
Mar 17, 2024
Merged

Adjust particle visuals #23

merged 6 commits into from
Mar 17, 2024

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
@@ -5,6 +5,7 @@
#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.

@@ -12,6 +12,7 @@ struct Styles : IService {
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.

@@ -48,16 +49,19 @@ void Game::on_loaded() {
m_player.foam_particles.set_texture(foam_texture);

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

@@ -3,11 +3,13 @@
#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/ui/button.hpp>
#include <spaced/ui/text.hpp>
#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?

@@ -22,6 +24,7 @@ Home::Home(bave::App& app, Services const& services) : Scene(app, services, "Hom
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.

2 participants