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.

Sign up for GitHub

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

Jump to bottom

Add blaster_BC_buttons #582

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add blaster_BC_buttons #582

wants to merge 12 commits into from

Conversation

NoSloppy
Copy link
Contributor

@NoSloppy NoSloppy commented Aug 1, 2023 •
edited
Loading

Needs fixes for dedicated power and reload buttons, esp if dual_prop used.
Guidance?

profezzorn
profezzorn reviewed Aug 13, 2023
View reviewed changes
Copy link
Owner

@profezzorn profezzorn left a comment

Choose a reason for hiding this comment

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

Just some initial comments.
IF this things were to inherit from the blaster prop, then there will be lots of changes, so I want to get that out of the way before I review in more detail.

common/saber_base.h
@@ -77,6 +77,7 @@ extern SaberBase* saberbases;
DEFINE_EFFECT(UNJAM) \
DEFINE_EFFECT(PLI_ON) \
DEFINE_EFFECT(PLI_OFF) \
DEFINE_EFFECT(DESTRUCT) \
Copy link
Owner

Choose a reason for hiding this comment

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

self-destruct is usually an off-type rather than an effect...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is,

        case NEXT_ACTION_BLOW:
          Off(OFF_BLAST);
          break;

But having EFFECT_DESTRUCT allows for blade animations.

props/blaster_BC_buttons.h Outdated
EFFECT(quote); // for playing quotes
#endif

class BlasterBC : public PROP_INHERIT_PREFIX PropBase {
Copy link
Owner

Choose a reason for hiding this comment

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

It can't inherit from the blaster prop?
(If not that's ok, but it should be considered if it will reduce code duplication)

Copy link
Contributor Author

@NoSloppy NoSloppy Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am unclear on this. To my understanding,
only one blaster prop file is included at a time, so the other isn't even compiled.
Isn't it just code substitution then as opposed to code duplication?
It would make sense if there were a blaster_base.h maybe, but there's not.

Copy link
Owner

Choose a reason for hiding this comment

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

The reason for avoiding code duplication is because code duplication makes the code hard to maintain.
(Basically, every time you change something you have to change it in multiple places.)
How much of the code is shown to the compiler is kind of irrelevant in that context.

Including another prop and inheriting from it is easy, but it can get messy if there is conflicting behavior.
Creating a blaster_base can resolve such problems, because then only the shared code goes in the base.
Can we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, here's a reverse diff reporting what's the same between blaster.h and my prop. 271 same lines.
Is this good/worth making a blaster_base for?
blaster_props_shared_code.txt

Copy link
Owner

Choose a reason for hiding this comment

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

yes, absolutely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, i'm modifying it now, assuming we're leaving it named blaster.h, not making a new file blaster_base.h, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. inheritance complete. How about now?

props/blaster_BC_buttons.h Outdated
PropBase::DoMotion(Vec3(0), clear);
}

RefPtr<BufferedWavPlayer> wav_player;
Copy link
Owner

Choose a reason for hiding this comment

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

variables should generally go at the bottom (and be indented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it above Event2 where I saw it in other props, and
I just added more bools to the spot that blaster.h has them.
So should the RefPtr (and all the other declarations) be moved to the bottom of the file?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@profezzorn
Copy link
Owner

Needs fixes for dedicated power and reload buttons, esp if dual_prop used.
Guidance?

It would be easier to provide guidance if I knew what the problem was...

@NoSloppy
Copy link
Contributor Author

NoSloppy commented Aug 24, 2023 •
edited
Loading

Needs fixes for dedicated power and reload buttons, esp if dual_prop used.
Guidance?

It would be easier to provide guidance if I knew what the problem was...

dual_prop doesn't account for these buttons I think was / is the issue.
How would that work if we already have a "power" button that gets swapped to a button named BUTTON_FIRE?
Would we just add the 2 additional non-remapped buttons something mostly like this?

#ifdef CONFIG_BUTTONS
Button PowerButton(BUTTON_POWER, powerButtonPin, "pow"); 
Button AuxButton(BUTTON_AUX, auxPin, "aux");
Button ReloadButton(BUTTON_RELOAD, aux2Pin, "reload");
Button ????Button (BUTTON_???? , 17, "???"); // using data3 on v2.2 or TX on v3.9 Proffieboards
#endif

profezzorn
profezzorn reviewed Mar 24, 2024
View reviewed changes
props/blaster_BC_buttons.h Outdated
@@ -649,7 +649,7 @@ void Loop() override {
}

// Blaster effects, auto fire is handled by begin/end lockup
void SB_Effect(EffectType effect, float location) override {
void SB_Effect2(EffectType effect, float location) {
Copy link
Owner

Choose a reason for hiding this comment

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

Still needs "override"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I see now what the issue was. It wasn't compiling, said it was marked override but didn't override. Seemed odd since it's overriding already in the Blaster class and we're inheriting from there. But I found it worked as SB_Effect2, so thought that was the right move.
The actual issue was the addition of EffectLocation since this was last worked on. changing float to EffectLocation makes things right now.
If I understand correctly, it only "needs" to be SB_Effect2 if we were trying to get sound lengths for example.

Copy link
Owner

Choose a reason for hiding this comment

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

Correct.
Generally SB_Effect() is the right place to play sounds, and SB_Effect2() is the right place to do something after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@profezzorn profezzorn profezzorn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NoSloppy @profezzorn