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

Added Bronco 40 amps, cabinets and effects #21

Open
wants to merge 2 commits into
base: 20-bronco
Choose a base branch
from

Conversation

StillVoidingWarranties
Copy link

I added data for Bronco 40 amps, cabinets and effects as good as I could and it kinda works. However it is quite messy and I broke the tests that require Mustang cabinets.

@@ -167,6 +178,7 @@ namespace plug

constexpr cabinets lookupCabinetById(std::uint8_t id)
{
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not comment out code, but add those values below (as with the amps).

@@ -29,8 +29,19 @@ namespace plug
// list of all amplifiers
enum class amps
{
FENDER_57_DELUXE,
// Bronco 40 amps
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not change the existing listing, but append to it's end.

};


// list of all effects
enum class effects
{
// Bronco 40 effects
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, please append and do not disable.

@@ -102,6 +113,7 @@ namespace plug
// list of all cabinets
enum class cabinets
{
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

@@ -183,6 +183,8 @@ namespace plug
// set properties
switch (static_cast<amps>(ampValue))
{

/*
Copy link
Owner

Choose a reason for hiding this comment

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

As above.

@@ -53,6 +53,8 @@
<property name="accessibleDescription">
<string>Allows you to choose amplifier to emulate</string>
</property>
Copy link
Owner

Choose a reason for hiding this comment

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

As above.

@@ -275,6 +275,10 @@ namespace plug
tr("When you choose an effect you can set precise value of a parameter here")});
break;
case effects::OVERDRIVE:
case effects::MODERN_BASS_OVERDRIVE:
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a Bronce comment to those so we can determine later.

@@ -51,11 +51,40 @@
<string>None</string>
</property>
</item>

<!-- Bronco 40 effects -->
Copy link
Owner

Choose a reason for hiding this comment

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

Remark: The .ui files are created by Qt Designer (usually) , so comments wont last long here. Having the comments in the source files is enough.

@@ -555,6 +572,10 @@ namespace plug
tr("When you choose an effect you can set precise value of a parameter here")});
break;
case effects::OVERDRIVE:
case effects::MODERN_BASS_OVERDRIVE:
Copy link
Owner

Choose a reason for hiding this comment

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

Bronce comment please.

@@ -10,7 +10,7 @@ target_link_libraries(TestLibs INTERFACE
add_subdirectory(mocks)



#[[
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not disable tests. It's fine for the prototype to have failing tests regarding this feature.

@offa
Copy link
Owner

offa commented Oct 24, 2023

However it is quite messy and I broke the tests that require Mustang cabinets.

Failing tests are ok at the moment. The first step is to figure out whether the device uses a compatible protocol (it seems so 🎉), figure out the ids and getting some working prototype that can do some communication with your device.

Once these are done we can work on the actual implementation, which requires some more work. The bronco dev branch wont get merged directly, but should diff easily to master, so we can follow the changes.

@offa
Copy link
Owner

offa commented Oct 24, 2023

Oh and thanks for your PR btw.! 👍

@StillVoidingWarranties
Copy link
Author

I first tried to just append the Bronco amps/cabs/effects but at some places the order seems to matter. Sorry, I'm not very familiar with C++ but I will clean up the code, test it manually and make another PR.

@offa
Copy link
Owner

offa commented Oct 24, 2023

I first tried to just append the Bronco amps/cabs/effects but at some places the order seems to matter.

That should happen, but if it does you have found a bug 🐛. Let me know if you need some help here.

Sorry, I'm not very familiar with C++

Really no problem, I can help where needed. Also it might help to do the changes step by step, eg. add amps, test, add cabinets etc.

and make another PR.

You can update this PR, just push commits to your branch.

@StillVoidingWarranties
Copy link
Author

Ok, will do.

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