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

Proposed additional button prop file for ProffieOS. #702

Closed
wants to merge 10 commits into from

Conversation

Sabersense-CC
Copy link

Proposed additional button prop file for ProffieOS by Chris at Sabersense Lightsabers.

This is a submission for an alternative prop file to be made available as part of ProffieOS.

OVERVIEW
Based on Matthew McGeary’s SA22C prop, my modifications have been made with simplicity in mind. It is not intended to be as full-featured as some more complex props - instead it is designed to be very easy to use, idiot-proof and consistent in terms of ‘rules’. It doesn’t require a ‘knack’ to access certain features, and tricky combinations like holding a button while simultaneously twisting or making other active gestures have been mostly eliminated. More complex features like Battle Mode that have the potential to confuse inexperienced users or tie them in a knot have also been removed.

HARMONIZED 1-BUTTON AND 2-BUTTON CONTROLS
Basic features like normal ignition, track playing, force effects, mute ignition, quotes etc. all have the same button controls on both 1-button and 2-button setups. This means users with large hilt collections which include 1-button and 2-button sabers don’t need to remember different sets of controls for these core features.

CONSISTENT ‘RULES’
This again means the user has less to remember. For example, one click of POWER with blade off ALWAYS lights the blade - short click for normal, long click for muted; double click POWER ALWAYS plays a sound effect - track or character quote with blade off, force effect or character quote with blade on pointed up/down respectively. This is all intended to make it easier for people to remember the various controls.

COMPREHENSIVE FONT NAVIGATION
1-button and 2-button setups both allow users to move forwards or backwards one font at a time, or to skip to first font, last font or middle font. 2-button setups include additional navigation features which allow the user to skip forwards or backwards 5 and 10 fonts at a time.

MANUAL AND FAKE BLADE ID
This allows users to run blade ID with a button press, eliminating potential blade ID scan errors that might occur when blades are lit. It can also be set with a define to toggle between two blade/preset arrays with a button press regardless of the actual BladeID status.

BUILT IN BUTTON ‘CLICKER’
For hilts that have buttons with poor tactile feedback (KR’s Scavenger for instance, with the emitter wheel button setup), simply adding a press.wav and release.wav of a clicking sound to the font, shared or common folder will play those effects with their respective button actions to aid in feature navigation.

Proposed additional button prop file for ProffieOS by Chris at Sabersense Lightsabers.
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.

First review pass.
There will be more....

props/saber_chris_buttons.h Outdated Show resolved Hide resolved
There are four gesture types: Twist, Stab, Swing and Thrust.
Gesture controls bypass all preon effects.
Below are the options to add to the config to enable the various gestures. MM.
#define SA22C_TWIST_ON
Copy link
Owner

Choose a reason for hiding this comment

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

These defines will need to be renamed since they apply to this prop, not SA22C now.

Copy link
Author

Choose a reason for hiding this comment

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

All now changed to SABERSENSE.

props/saber_chris_buttons.h Outdated Show resolved Hide resolved
EFFECT(blstbgn); // for Begin Multi-Blast
EFFECT(blstend); // for End Multi-Blast
EFFECT(push); // for Force Push gesture in Battle Mode
EFFECT(quote); // for playing quotes
Copy link
Owner

Choose a reason for hiding this comment

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

If I remember correctly, this is in hybrid_font.h now...

Copy link
Author

Choose a reason for hiding this comment

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

I had a look and I can't see this extract in hybrid_font.h. Also I tried removing this EFFECT list from the prop, but the result was that it wouldn't compile. What have I missed?

Copy link
Owner

Choose a reason for hiding this comment

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

I didn't mean the whole list, just EFFECT(quote)

Copy link
Author

@Sabersense-CC Sabersense-CC Nov 17, 2024

Choose a reason for hiding this comment

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

Ah sorry.
Noting your comment that this would not potentially go live until OS-8, I can see what's happened now - the quote line is indeed handled in hybrid_font.h in OS-8 (GitHub Master) but not OS-7. Apologies - all my testing has been done in OS-7.14, in which case that line is needed.

I'll test everything today in OS-8, but early indications are that there are two issues between OS-7 and OS-8:

The first is the EFFECT(quote); line above which we now know about.

The second is on line 969.
OS-7 seems to want this:
void SB_Effect(EffectType effect, float location) override {

But OS-8 wants this:
void SB_Effect(EffectType effect, EffectLocation location) override {

Neither OS will compile with the wrong line.

I could put them both on an #ifndef define so we're at least backwards compatible with OS-7. Or are we not worried about that?

The rest of the formatting stuff I'm working through now. Will upload the (hopefully) correct version later today.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, and thanks for your patience with this - much appreciated as always.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sabersense-CC Just FYI - Lots of updated spots through the OS will use EffectLocation for location now.
Backwards compatibility is not typically a thing with prop files. They will move forward and be updated if needed, but I've not seen one go backwards to work with a previous version from when they were added.
That said, I think you should be using the master version here for development.

Copy link
Author

Choose a reason for hiding this comment

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

...Backwards compatibility is not typically a thing with prop files....

Thanks Brian. All noted. I had a hunch that was the case, but the main reason I've been using 7.14 for the time being is because colour change didn't seem to work right under the Master version, despite the fact I haven't made any changes to any colour change stuff. Hence I wanted to get everything working on a stable system first, and then make the necessary tweaks to make it work with OS-8.

Copy link
Contributor

@NoSloppy NoSloppy Dec 1, 2024

Choose a reason for hiding this comment

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

Colorchange works, it's just been moved to a mode. That means that once you're in that mode, you are essentially isolated from your prop file controls until you exit, which is nice because you can avoid interference with other things in your prop without having to guard everything. The mode has its own set of button controls, and the default are click POW to save, double click POW to cancel. You can customize that if you want. I suppose you could think of modes as sort of mini prop files.

Copy link
Author

Choose a reason for hiding this comment

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

Colorchange works, it's just been moved to a mode....
Aha! I wasn't aware of that. Yes, just tried it and all good. 👍

props/saber_chris_buttons.h Outdated Show resolved Hide resolved
props/saber_chris_buttons.h Outdated Show resolved Hide resolved
props/saber_chris_buttons.h Outdated Show resolved Hide resolved
props/saber_chris_buttons.h Outdated Show resolved Hide resolved
File renamed for consistent naming convention (Sabersense) throughout as requested.
Sorry - forgot one little detail on Battery Level Check. Now sorted.
Pretty sure I just swatted the last bug. Good to go now. :)
// Used instead of BladeID and allows toggle switching
// between two blade/preset arrays manually irrespective
// of actual BladeID status.
#ifdef SABERSENSE_FAKE_BLADE_ID
Copy link
Owner

Choose a reason for hiding this comment

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

207 to 220 are super-weirdly indented, please fix.

Copy link
Author

Choose a reason for hiding this comment

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

Hopefully now sorted.

props/saber_chris_buttons.h Outdated Show resolved Hide resolved
EFFECT(quote); // for playing quotes



Copy link
Owner

Choose a reason for hiding this comment

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

extra empty line

Copy link
Author

Choose a reason for hiding this comment

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

Done.

}
}

// Volume Menu
Copy link
Owner

Choose a reason for hiding this comment

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

indent comment two steps

Copy link
Author

Choose a reason for hiding this comment

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

Hoping the formatting is now OK as I've removed all the tabs and and hopefully changed it to preferred convention.

}


// Button Clicker to play press/release wav files when buttons are pressed.
Copy link
Owner

Choose a reason for hiding this comment

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

indent comment two steps

Copy link
Author

Choose a reason for hiding this comment

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

Done for all comments.

if (!player->PlayInCurrentDir(sound)) player->Play(sound);
}
}
bool Event2(enum BUTTON button, EVENT event, uint32_t modifiers) override {
Copy link
Owner

Choose a reason for hiding this comment

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

I can't count spaces in the review tool, but PlaySound and Event2 are definitely not indented the same, but I think they should be. (2 steps)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.



// Gesture Controls
#ifdef SABERSENSE_SWING_ON
Copy link
Owner

Choose a reason for hiding this comment

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

preprocessor statements should generally not be indented

Copy link
Author

Choose a reason for hiding this comment

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

Sorted.

return true;


// Gesture Controls
Copy link
Owner

Choose a reason for hiding this comment

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

comments should be indented

Copy link
Author

Choose a reason for hiding this comment

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

Done.


// Gesture Controls
#ifdef SABERSENSE_SWING_ON
case EVENTID(BUTTON_NONE, EVENT_SWING, MODE_OFF):
Copy link
Owner

Choose a reason for hiding this comment

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

Almost everything from here on is indented weird.
I suggest using a formatting tool, configure it for google rules with 2-space indents (no tabs) and re-format all of it.

Copy link
Author

@Sabersense-CC Sabersense-CC Nov 17, 2024

Choose a reason for hiding this comment

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

Went through it all manually and deleted tabs, replacing them with appropriate spaces, hopefully to preferred convention.

Hopefully the formatting is now correct.
However there is an issue with colour change under GitHub Master (OS-8?) which wasn't present under OS-7. As such I don't know if it's the prop or something else. (It it's the prop, then I'm probably defeated as I wouldn't know how to fix it).
Specifically:

1-button
Exiting colour change needs two clicks despite three clicks being defined in the prop. (I don't know where it gets two-clicks from).
Colour reverts to original colour when exiting menu.

2-button
Colour reverts to original colour when exiting menu.
Just spotted an error. Now sorted.
Fixed bug where absence of quote.wav would prevent force and track wavs playing.
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.

There are a lot of indentation issues, please apply these comments wherever you see similar issues, because I haven't pointed out every single occurrence. (That would take all day.)

RefPtr<BufferedWavPlayer> player = GetFreeWavPlayer();
if (player) {
if (!player->PlayInCurrentDir(sound)) player->Play(sound);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Indentation doesn't like up.

Generally, the "}" should line up with the start, sort of like this:

    if (something) {
      /* code */
    }   // This has the same amount of indentation as the if statement that started the block two lines up

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip - hopefully now sorted throughout the file.

}
bool Event2(enum BUTTON button, EVENT event, uint32_t modifiers) override {
switch (EVENTID(button, event, modifiers)) {
case EVENTID(BUTTON_POWER, EVENT_PRESSED, MODE_ANY_BUTTON | MODE_ON):
Copy link
Owner

Choose a reason for hiding this comment

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

case statements should be indented more than the switch they are in.
(Every statement should be indented more than the block they are in.)

Copy link
Author

Choose a reason for hiding this comment

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

Hopefully now sorted.

pointing_down_ = true;
} else {
pointing_down_ = false;
}
Copy link
Owner

Choose a reason for hiding this comment

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

end } doesn't line up

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

// Due to motion chip startup on boot creating false ignition we delay Swing On at boot for 3000ms
if (millis() > 3000) {
FastOn();
// On();
Copy link
Owner

Choose a reason for hiding this comment

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

Why keep this here? Just remove it.

Copy link
Author

Choose a reason for hiding this comment

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

I've gone through and deleted all old and redundant code, including Battle Mode which I don't intend to include in this prop.

// Delay twist events to prevent false trigger from over twisting
if (millis() - last_twist_ > 3000) {
Off();
last_twist_ = millis();
Copy link
Owner

Choose a reason for hiding this comment

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

the indentation of this statement doesn't line up with the indentation of the previous one.

Copy link
Author

Choose a reason for hiding this comment

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

Missed that. Now sorted.

case EVENTID(BUTTON_AUX, EVENT_SECOND_SAVED_CLICK_SHORT, MODE_OFF):
// backwards if pointing down
SetPreset(current_preset_.preset_num + (fusor.angle1() < -M_PI / 4 ? -5 : 5), true);
#ifdef SAVE_PRESET
Copy link
Owner

Choose a reason for hiding this comment

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

preprocessor directives should not be indented

Copy link
Author

@Sabersense-CC Sabersense-CC Nov 24, 2024

Choose a reason for hiding this comment

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

Sorry - I missed that one too. Now fixed, and I've checked all the others too.

return true;
#endif

// Multiple Skips only available with 2 button installs.
Copy link
Owner

Choose a reason for hiding this comment

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

Comments should be indented to match the next line of code.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I think I've done this right now.
I've basically done two spaces in for a comment starting a new section, unless the comment comes before a case in which case it's four spaces.

VolumeUp();
} else {
VolumeDown();
}
Copy link
Owner

Choose a reason for hiding this comment

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

indent doesn't match

Copy link
Author

Choose a reason for hiding this comment

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

It does now, hopefully in all instances..

#ifdef SAVE_PRESET
SaveState(current_preset_.preset_num);
#endif
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't this return statement indented properly (should match previous line generally)

Copy link
Author

Choose a reason for hiding this comment

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

Sorted.

Updated and (hopefully) corrected indenting.
More redundant lines removed and tested. Since I'm not including Battle Mode, I've removed all references to it. Likewise the Sabersense defines that were listed near the top of the config - it turns out they didn't need to be there - they still work just appearing in the relevant places in the code. 
As such, hopefully it's a little tidier now. :)
A few more I missed...
@Sabersense-CC
Copy link
Author

All recommended changes have been made, but I've also done a lot of tidying, re-arranging and adding of new stuff, so I figured it would be easier to close this request and open a new one with my complete, finished prop. Hope that's OK.

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