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

[CPP/LUA] Add synthesis high quality chance configurable setting #5712

Merged

Conversation

ampitere
Copy link
Contributor

@ampitere ampitere commented May 13, 2024

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Adds synthesis high quality chance configurable setting requested in #5453

Steps to test these changes

  1. Tweak the setting
  2. Attempt to synth or desynth

@ampitere
Copy link
Contributor Author

Will do some more testing on this tomorrow, created as a draft for now.

@Xaver-DaRed
Copy link
Contributor

Xaver-DaRed commented May 13, 2024

Several things here:

  • Synthesis configurations should all remain together. This should go bellow the existing ones.
  • They should all follow the same naming convention. This should all start with CRAFT_
  • I think this is absolutely overkill. Current configurations are there because once upon a time, the system worked with different caps and different rates (era vs modern). HQ rates have never changed, I dont think EVERY single number needs a configuration.

I would limit it to a single HQ chance multiplier. That would seem more reasonable.

settings/default/map.lua Outdated Show resolved Hide resolved
src/map/utils/synthutils.cpp Outdated Show resolved Hide resolved
@ampitere ampitere force-pushed the add_synthesis_hq_related_settings branch 3 times, most recently from 4344aa1 to 5e4eb89 Compare May 13, 2024 16:13
@ampitere
Copy link
Contributor Author

ampitere commented May 13, 2024

Several things here:

  • Synthesis configurations should all remain together. This should go bellow the existing ones.
  • They should all follow the same naming convention. This should all start with CRAFT_
  • I think this is absolutely overkill. Current configurations are there because once upon a time, the system worked with different caps and different rates (era vs modern). HQ rates have never changed, I dont think EVERY single number needs a configuration.

I would limit it to a single HQ chance multiplier. That would seem more reasonable.

Refactored into a single setting that is an additional multiplier applied to chanceHQ.

@ampitere ampitere force-pushed the add_synthesis_hq_related_settings branch from 5e4eb89 to ac379cc Compare May 13, 2024 16:17
@ampitere ampitere changed the title [CPP/LUA] Add synthesis high quality related configurable settings [CPP/LUA] Add synthesis high quality chance configurable setting May 13, 2024
@ampitere ampitere marked this pull request as ready for review May 13, 2024 16:34
@ampitere ampitere force-pushed the add_synthesis_hq_related_settings branch from ac379cc to 12bdff7 Compare May 13, 2024 18:00
@ampitere ampitere force-pushed the add_synthesis_hq_related_settings branch from 12bdff7 to 96e131c Compare May 13, 2024 18:02
Copy link
Contributor

@Xaver-DaRed Xaver-DaRed left a comment

Choose a reason for hiding this comment

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

Beautiful

@claywar claywar merged commit cdbc28b into LandSandBoat:base May 14, 2024
11 checks passed
@ampitere ampitere deleted the add_synthesis_hq_related_settings branch May 14, 2024 14:54
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.

None yet

4 participants