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.

Sign up for GitHub

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

Jump to bottom

add allowHomonymObjectName option for OBJModel.ModelSettings #9047

Open
wants to merge 1 commit into
base: 1.19.x
Choose a base branch
from
Open

add allowHomonymObjectName option for OBJModel.ModelSettings #9047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/net/minecraftforge/client/model/obj/ObjLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public ObjModel read(JsonObject jsonObject, JsonDeserializationContext deseriali
boolean flipV = GsonHelper.getAsBoolean(jsonObject, "flip_v", false);
boolean emissiveAmbient = GsonHelper.getAsBoolean(jsonObject, "emissive_ambient", true);
String mtlOverride = GsonHelper.getAsString(jsonObject, "mtl_override", null);
boolean allowHomonymObjectName = GsonHelper.getAsBoolean(jsonObject,"allow_homonym_object_name",false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting: spaces after commas.

Suggested change
boolean allowHomonymObjectName = GsonHelper.getAsBoolean(jsonObject,"allow_homonym_object_name",false);
boolean allowHomonymObjectName = GsonHelper.getAsBoolean(jsonObject, "allow_homonym_object_name", false);


// TODO: Deprecated names. To be removed in 1.20
var deprecationWarningsBuilder = ImmutableMap.<String, String>builder();
Expand Down Expand Up @@ -86,7 +87,7 @@ public ObjModel read(JsonObject jsonObject, JsonDeserializationContext deseriali
deprecationWarningsBuilder.put("materialLibraryOverride", "mtl_override");
}

return loadModel(new ObjModel.ModelSettings(new ResourceLocation(modelLocation), automaticCulling, shadeQuads, flipV, emissiveAmbient, mtlOverride), deprecationWarningsBuilder.build());
return loadModel(new ObjModel.ModelSettings(new ResourceLocation(modelLocation), automaticCulling, shadeQuads, flipV, emissiveAmbient, mtlOverride,allowHomonymObjectName), deprecationWarningsBuilder.build());
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting: spaces after commas.

Suggested change
return loadModel(new ObjModel.ModelSettings(new ResourceLocation(modelLocation), automaticCulling, shadeQuads, flipV, emissiveAmbient, mtlOverride,allowHomonymObjectName), deprecationWarningsBuilder.build());
return loadModel(new ObjModel.ModelSettings(new ResourceLocation(modelLocation), automaticCulling, shadeQuads, flipV, emissiveAmbient, mtlOverride, allowHomonymObjectName), deprecationWarningsBuilder.build());

}

public ObjModel loadModel(ObjModel.ModelSettings settings)
Expand Down
23 changes: 22 additions & 1 deletion src/main/java/net/minecraftforge/client/model/obj/ObjModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ static ObjModel parse(ObjTokenizer tokenizer, ModelSettings settings, Map<String
{
var modelLocation = settings.modelLocation;
var materialLibraryOverrideLocation = settings.mtlOverride;
var allowHomonymObjectName = settings.allowHomonymObjectName;
var hasHomonymObjectNameAppeared = false;
var model = new ObjModel(settings, deprecationWarnings);

// for relative references to material libraries
Expand Down Expand Up @@ -280,6 +282,21 @@ static ObjModel parse(ObjTokenizer tokenizer, ModelSettings settings, Map<String
case "o":
{
String name = line[1];
if (model.parts.containsKey(name))
{
hasHomonymObjectNameAppeared = true;
if (allowHomonymObjectName)
{
int suffixNum = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should begin this at 1 instead, so the names would be: object, object_1, object_2.

String renamed;
do
{
renamed = name + '_' + (suffixNum++);
}
while (model.parts.containsKey(renamed));
name = renamed;
}
}
if (objAboveGroup || currentGroup == null)
{
objAboveGroup = true;
Expand All @@ -299,6 +316,10 @@ static ObjModel parse(ObjTokenizer tokenizer, ModelSettings settings, Map<String
}
}
}
if (!allowHomonymObjectName && hasHomonymObjectNameAppeared)
{
LOGGER.error("find homonym object in obj model:" + settings.modelLocation + ", use allow_homonym_object_name=true can rename them for you");
Copy link
Contributor

Choose a reason for hiding this comment

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

Some error message cleanup, as well as using logger parameter substitution instead of manual string concatenation:

Suggested change
LOGGER.error("find homonym object in obj model:" + settings.modelLocation + ", use allow_homonym_object_name=true can rename them for you");
LOGGER.error("Found homonym object in ObjModel: {}; use the 'allow_homonym_object_name' option to automatically resolve homonyms", settings.modelLocation);

}
return model;
}

Expand Down Expand Up @@ -687,6 +708,6 @@ public void bake(CompositeRenderable.PartBuilder<?> builder, IGeometryBakingCont

public record ModelSettings(@NotNull ResourceLocation modelLocation,
boolean automaticCulling, boolean shadeQuads, boolean flipV,
boolean emissiveAmbient, @Nullable String mtlOverride)
boolean emissiveAmbient, @Nullable String mtlOverride, boolean allowHomonymObjectName)
{ }
Comment on lines 709 to 712
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change because any callers to the previous constructor (without the new parameter) will break.

Please add a new constructor to this record which calls the canonical constructor with an appropriate default value for the new parameter, mark that new constructor as deprecated for removal (@Deprecated(forRemoval = true, since = "1.19.2"), and add a javadoc with a @deprecated tag directing users to the canonical constructor (which has the new parameter).

}
3 changes: 2 additions & 1 deletion src/test/java/net/minecraftforge/debug/client/rendering/RenderableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ protected ObjModel prepare(ResourceManager resourceManager, ProfilerFiller profi
true,
true,
false,
null
null,
false
);
return ObjLoader.INSTANCE.loadModel(settings);
}
Expand Down