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

make game agnostic, add luacheck, add github actions #5

Merged
merged 3 commits into from
Feb 24, 2022
Merged

Conversation

wsor4035
Copy link
Contributor

see title

@BuckarooBanzay
Copy link
Member

didn't test but looks good to me 👍

also:

local newcolor = "razzberry" -- intentionally misspelled ;-)

why was that there anyway? 🤔

@BuckarooBanzay BuckarooBanzay added the enhancement New feature or request label Feb 23, 2022
airbrush.lua Show resolved Hide resolved
.luacheckrc Show resolved Hide resolved
@OgelGames OgelGames added the bug Something isn't working label Feb 23, 2022
@OgelGames
Copy link
Contributor

OgelGames commented Feb 23, 2022

Tested this using devtest (no dye mod), and some of the colors cannot be selected when using the airbrush, e.g. white, grey and black.

I think there needs to be code added to register the dyes from the dye mod if it's not installed.

@wsor4035
Copy link
Contributor Author

Tested this using devtest (no dye mod), and some of the colors cannot be selected when using the airbrush, namely white, grey and black.

might be related to https://github.com/mt-mods/unifieddyes/pull/5/files#diff-51f25c75e72ab95273185b8033d3f18c8d35ac5258cada768ffca5757d4c5429L90-R104 ill have to investigate further. what node did you test dying?

I think there needs to be code added to register the dyes from the dye mod if it's not installed.

only whats needed should be registered, as different games may/do have there own dyes

@OgelGames
Copy link
Contributor

what node did you test dying?

Plastic (scifi_nodes:white2), which supports all colors.

BTW, I should mention it's not just white, grey and black, it's other colors too, I just didn't bother checking all 256 colors ;)

@wsor4035
Copy link
Contributor Author

Tested this using devtest (no dye mod), and some of the colors cannot be selected when using the airbrush, e.g. white, grey and black.

608c806 resolved

@OgelGames
Copy link
Contributor

OgelGames commented Feb 24, 2022

That fixes it for creative, but not for survival, because you need the dye in your inventory.

IMO there are two ways to fix this: make dye a dependency, or register the other dyes, preferably the latter.

only whats needed should be registered, as different games may/do have there own dyes

They are needed, for crafting recipes and for using the airbrush in survival.

@wsor4035
Copy link
Contributor Author

wsor4035 commented Feb 24, 2022

That fixes it for creative, but not for survival, because you need the dye in your inventory.

Thanks for testing/confirming this

IMO there are two ways to fix this: make dye a dependency, or register the other dyes, preferably the latter.

the point is to make it game agnostic, dye is mtg. this mod still works as intended in minetest game, and adds support for other games in creative. A additional PR can add support for survival later.

only whats needed should be registered, as different games may/do have there own dyes

They are needed, for crafting recipes and for using the airbrush in survival.

no, not really

@OgelGames
Copy link
Contributor

A additional PR can add support for survival later.

No, it should be added here.

@wsor4035
Copy link
Contributor Author

A additional PR can add support for survival later.

No, it should be added here.

It really doesnt, as it purely doesnt allow you to use it in survival in other games. not broken/crashes/etc. so not essential

@wsor4035 wsor4035 removed the bug Something isn't working label Feb 24, 2022
@wsor4035
Copy link
Contributor Author

to clarify, as with any game agnostic mod, crafting of the items within it can only be done defined crafts for certain games.

@OgelGames OgelGames added the bug Something isn't working label Feb 24, 2022
@BuckarooBanzay
Copy link
Member

BuckarooBanzay commented Feb 24, 2022

Can we, if possible, continue the discussion about the overall functionality in #8
This can be merged IMO as it just decouples the default dep and doesn't add or remove functionality.
@OgelGames can you live with that?

@OgelGames
Copy link
Contributor

It should be okay to leave it slightly broken for a bit, as it doesn't break anything for current users 👍

@wsor4035 wsor4035 removed the bug Something isn't working label Feb 24, 2022
@wsor4035 wsor4035 merged commit 41dd54d into master Feb 24, 2022
@wsor4035 wsor4035 deleted the luacheck branch February 24, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants