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

The limitation of attributes; Deform proportionally on the existing scale. #12

Merged
merged 11 commits into from
Sep 27, 2024

Conversation

Noogear
Copy link
Contributor

@Noogear Noogear commented Sep 24, 2024

I noticed a problem. When generating the configuration file, the keys without values will be ignored, and the "scale:" in the last line has no value, so it will not be generated. I'm very sorry that I don't know how to modify it.

@Noogear
Copy link
Contributor Author

Noogear commented Sep 24, 2024

This PR is mainly for the implementation of this issue: #10 .

@ErikSzabo
Copy link

You have created a breaking change in how scaling works for no real reason. Either remove the change or make it backwards compatible and configurable.

@Noogear
Copy link
Contributor Author

Noogear commented Sep 25, 2024

You have created a breaking change in how scaling works for no real reason. Either remove the change or make it backwards compatible and configurable.

The reason is Deform proportionally on the existing scale. This will not cause any destructive impact because the default scale of all creatures is 1.0, and 1.0 * random will not have any effect. For those that are not 1.0, the scale after the proportional change will be set, that is, the original scale * random. Wouldn't the scale of your custom creature being directly overridden by the plugin be more destructive? But if you need it, I will modify it, that is, to allow the plugin to forcibly override the scale of your custom creature instead of scaling it up or down proportionally.

@ErikSzabo
Copy link

Hmm, you got a point there. @Fly1337 what do you think?

@Fly1337
Copy link
Contributor

Fly1337 commented Sep 25, 2024

In my opinion, forcefully overriding an existing feature which was implemented already, is about never a good option in terms of public plugins, even though in this case, there is no official release jar yet. I think, that forcefully setting a size as well as setting it in a relative way, have both their points, but then, they would have to exist both in a way, where both respect each other. I'd say, that the way to go would be either an indicator, for example "r1-2" for relative and "1-2" for absolute, or even its own config entry, stating absolute or relative.

@Noogear
Copy link
Contributor Author

Noogear commented Sep 25, 2024

In my opinion, forcefully overriding an existing feature which was implemented already, is about never a good option in terms of public plugins, even though in this case, there is no official release jar yet. I think, that forcefully setting a size as well as setting it in a relative way, have both their points, but then, they would have to exist both in a way, where both respect each other. I'd say, that the way to go would be either an indicator, for example "r1-2" for relative and "1-2" for absolute, or even its own config entry, stating absolute or relative.

Because the current configuration file has problems. Just as I said at the beginning, "scales: " will disappear in the configuration file. If you want to comply with the specifications of the original code, the changes to the code will be very huge, which is not something I can simply achieve.
If I want to conform to the original specifications, it should be changed as follows.

scales:
  cover: false
  level:
    1-100:
      chance: 0.1
      scale: 0.8-1.2

And this "level: " cannot be left blank. It must be followed by [] or {}, otherwise, if it is empty, it will be eliminated by the configuration loading method. This is a very huge project and it is very difficult for me to accomplish.I'm not saying it can't be done, but the code after completion will be non-compliant and rough. If you can accept that, then let me do it.

Copy link
Owner

@Archy-X Archy-X left a comment

Choose a reason for hiding this comment

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

Please make the formatting changes requested

@Noogear Noogear requested a review from Archy-X September 26, 2024 01:40
@Noogear Noogear requested a review from Archy-X September 26, 2024 12:34
@Noogear
Copy link
Contributor Author

Noogear commented Sep 26, 2024

Now users can choose whether to directly override the original scale or scale it up or down proportionally, (config.yml - scales.cover)

@Archy-X Archy-X merged commit 2dd6ab8 into Archy-X:main Sep 27, 2024
1 check passed
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.

4 participants