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

Sluuuurp #231

Merged
merged 236 commits into from
Sep 20, 2023
Merged

Sluuuurp #231

merged 236 commits into from
Sep 20, 2023

Conversation

jrddunbr
Copy link
Contributor

This should be merged sooner than later.

empireu and others added 30 commits January 24, 2023 17:18
…to CellBlockEntity (which is the more correct name for it). Also added some utility extensions for block entities.
…e prefix "on". Also renamed some methods to more appropriate names.
…the existing entity or creating one, and placing the part on the face.
…d added a whole load of mathematics APIs (which were needed for the shape geometrical calculations), operator overloads and utilities for Minecraft's vectors and matrices
…st enqueue relights whenever they wanted. Now returning models to relight in the IPartRenderer.relightModels method
empireu and others added 26 commits May 23, 2023 20:15
… and adds automatic subscriber cleanup to cells
… on ArcReparamCatenary2dRFUProjection3dDual)
Polishes core by implementing testing bed
Copy link
Contributor Author

@jrddunbr jrddunbr left a comment

Choose a reason for hiding this comment

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

Yeah, I mean, I left a few comments but I'm straight up going to merge this after I comment. You basically rewrote the mod, and comments I made can be addressed in further pull requests that are actually digestible. (Nothing looks like it's a major concern, just some various lints here and there). I didn't playtest it and honestly I don't think I'll fail merge on that since this is such a large change and it just needs to be merged already.

Comment on lines +10 to +36
/* fun loadConfig(configFile: File = CONFIG_FILE) {
try {
if (configFile.isFile) {
LOGGER.info("[Electrical Age] Reading config from ${configFile.absoluteFile}")
instance = Yaml.default.decodeFromStream(ElnConfig.serializer(), configFile.inputStream())
} else {
instance = ElnConfig()
saveConfig()
}
} catch (e: Exception) {
LOGGER.error("Electrical Age had an issue with loading the configuration file, please check the file for errors.")
LOGGER.error("Check that 1) You have valid YAML 2) the config directives are spelled correctly (see documentation)")
}
}

private fun saveConfig(configFile: File = CONFIG_FILE) {
try {
LOGGER.info("[Electrical Age] Writing config to ${configFile.absoluteFile}")
val configText = Yaml.default.encodeToString(ElnConfig.serializer(), instance)
if (!configFile.exists()) {
configFile.createNewFile()
}
configFile.writeText(configText)
} catch (e: Exception) {
LOGGER.error("Electrical Age was unable to write the config file, please check filesystem permissions: $e")
}
}*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit. Not sure why this is commented. Not really an issue but hey.

fun getResourceString(location: ResourceLocation): String =
getResourceStream(location).readAllBytes().toString(Charset.defaultCharset())

val GAME = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's this used for? Probably could use a comment.

inline fun <reified T : Cell> Level.getCell(mb: MultiblockManager, cellPosId: BlockPos): T =
getCellOrNull(mb, cellPosId) ?: error("Cell was not present")

private const val LIBAGE_SET_EPS = 0.001
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bunch of libage extensions here (below) that might make sense as part of libage? Might want to move them, in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that, or move to libage.kt?

return this.translate(Vec3(0.5, 0.0, 0.5))
}

private const val NBT_ELECTRICAL_RESISTIVITY = "electricalResistivity"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the below NBT stuff I think could warrant it's own file as NBTExtensions.kt or something?

@@ -0,0 +1,50 @@
package org.eln2.mc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we had a linter, it would probably loose it's marbles on this file.

@@ -0,0 +1,2237 @@
@file:Suppress("LocalVariableName")

package org.eln2.mc.mathematics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a big file. consider splitting it up and make a package level for it.

@@ -14,7 +14,7 @@ displayURL="https://age-series.org/"
# A file name (in the root of the mod JAR) containing a logo for display
logoFile=""
credits="The Age Series Team"
authors="jrddunbr, Caeleron, Grissess, Baughn, empireu, DarthSidiousPalpatine, AlanFletes, bloxgate"
authors="jrddunbr, Caeleron, Grissess, Baughn, DarthSidiousPalpatine, AlanFletes, bloxgate"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

writes code change. also removes self from authors list

Seriously, dude. You wrote more of the Eln2 mod than any of the people in this list except maybe Grissess.

@@ -1,11 +1,11 @@
modLoader="kotlinforforge"
loaderVersion="[3,)"
modLoader="javafml"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we ditched kff?

Comment on lines +148 to +149
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read the tests and I think that we could improve them if we added mockito

@@ -0,0 +1,1542 @@
package org.eln2.mc.common.cells.foundation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

good candidate for it's own package and split into files

@jrddunbr jrddunbr marked this pull request as ready for review September 20, 2023 01:40
@jrddunbr jrddunbr merged commit d57f404 into age-series:dev Sep 20, 2023
1 of 2 checks passed
@jrddunbr jrddunbr mentioned this pull request Sep 20, 2023
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.

3 participants