Separate plugin types #10

Merged
owlsys merged 14 commits from ecorous/plugin-types into main 2024-06-16 17:13:54 -04:00
Owner

very buggy - needs a lot of work

very buggy - needs a lot of work
Ecorous added 1 commit 2024-06-13 17:09:33 -04:00
Ecorous added 1 commit 2024-06-14 09:58:45 -04:00
owlsys added 2 commits 2024-06-15 09:17:00 -04:00
owlsys added 1 commit 2024-06-15 10:57:30 -04:00
owlsys changed title from WIP: initial work on separating plugin types to initial work on separating plugin types 2024-06-15 10:58:32 -04:00
owlsys requested review from Owners 2024-06-15 10:58:38 -04:00
owlsys added 1 commit 2024-06-15 13:59:08 -04:00
(since 1.21 isn't valid semver but we can expand it)
owlsys added 1 commit 2024-06-15 14:34:35 -04:00
# Conflicts:
#	minecraft/build.gradle.kts
owlsys added 1 commit 2024-06-15 14:40:04 -04:00
owlsys added 1 commit 2024-06-15 15:12:40 -04:00
owlsys changed title from initial work on separating plugin types to Separate plugin types 2024-06-15 16:45:57 -04:00
owlsys added 1 commit 2024-06-15 16:47:25 -04:00
owlsys added 1 commit 2024-06-16 06:26:28 -04:00
owlsys added 1 commit 2024-06-16 08:58:38 -04:00
Ecorous reviewed 2024-06-16 12:15:03 -04:00
Ecorous left a comment
Author
Owner

Looks good for the most part.

Looks good for the most part.
@ -15,3 +15,3 @@
mixin = "example_mod.mixins.json"
accesswidener = "example_mod.accesswidener"
modprovider = "dev.frogmc.frogloader.example.ExampleModProvider"
Author
Owner

Should we allow more than one modprovider per frogmod?

Should we allow more than one modprovider per frogmod?
Owner

Already possible

Already possible
Ecorous marked this conversation as resolved
@ -0,0 +21,4 @@
* Get the ID of this provider. Mods will be stored separated by their provider ID.
* @return The ID of this provider
*/
String id();
Author
Owner

Should/could this be changed to a ResourceLocation?

Should/could this be changed to a ResourceLocation?
Owner

No, thought about it but ResourceLocation is a Minecraft class and therefore may not be available.

No, thought about it but ResourceLocation is a Minecraft class and therefore may not be available.
Author
Owner

Should we make a similar wrapper class? It seems strange not to enforce the used format

Should we make a similar wrapper class? It seems strange not to enforce the used format
Owner

I don't think so, it doesn't really matter. The only important thing is that the content is unique among all mod providers due to the constraints of java's HashMap. And as we aren't really doing anything with these IDs except for supplying each provider with its mods later on I don't think it's worth requiring any specific format.

I don't think so, it doesn't really matter. The only important thing is that the content is unique among all mod providers due to the constraints of java's HashMap. And as we aren't really doing anything with these IDs except for supplying each provider with its mods later on I don't think it's worth requiring any specific format.
Author
Owner

It's something to consider if we ever want to parse it

It's something to consider if we ever want to parse it
Ecorous marked this conversation as resolved
owlsys added 1 commit 2024-06-16 13:24:39 -04:00
kode reviewed 2024-06-16 15:43:29 -04:00
@ -0,0 +2,4 @@
import dev.frogmc.frogloader.api.plugin.ModProvider;
public class ExampleModProvider implements ModProvider {
Owner

I'm asumming this serves as a code example - in which case it should be moved to the documentation

I'm asumming this serves as a code example - in which case it should be moved to the documentation
Author
Owner

The entire minecraft project is a test project - this tests that the discovery works properly

The entire `minecraft` project is a test project - this tests that the discovery works properly
Author
Owner

Some of this should be in the documentation as well, but this is fine to stay

Some of this should be in the documentation as well, but this is fine to stay
Ecorous marked this conversation as resolved
@ -0,0 +25,4 @@
List<ModProvider> providers = new ArrayList<>(ServiceLoader.load(ModProvider.class).stream().map(ServiceLoader.Provider::get).toList()); // we need mutability & random access
Map<String, Collection<ModProperties>> properties = new HashMap<>();
AtomicInteger size = new AtomicInteger(providers.size()); // use a separate size variable to not have to iterate over the list over and over again
Owner

wouldn't int[] size = new int[] { providers.size() } be better as there's no need for thread safety?

wouldn't `int[] size = new int[] { providers.size() }` be better as there's no need for thread safety?
Author
Owner

I believe this works better - but I wouldn't know as I didn't write it

I believe this works better - but I wouldn't know as I didn't write it
Author
Owner

The only difference here would be a insignificantly smaller memory footprint - this isn't an issue

The only difference here would be a insignificantly smaller memory footprint - this isn't an issue
Owner

It's not about performance, i think it's bad practice to use AtomicInteger when you don't need it

It's not about performance, i think it's bad practice to use AtomicInteger when you don't need it
Author
Owner

Resolved in 39ba054fcb

Resolved in 39ba054fcbb139134081d9bf776e035c4d893182
Ecorous marked this conversation as resolved
owlsys added 1 commit 2024-06-16 16:44:58 -04:00
ender approved these changes 2024-06-16 17:10:50 -04:00
owlsys merged commit 06b28c6e16 into main 2024-06-16 17:13:54 -04:00
owlsys deleted branch ecorous/plugin-types 2024-06-16 17:13:55 -04:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: frogmc/FrogLoader#10
No description provided.