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

Fixes #2114 (nulls in LodGenerator result) #2117

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

MelVimL
Copy link
Contributor

@MelVimL MelVimL commented Oct 16, 2023

This patch fixes 2114 and adds simple tests.

Removes the check if some lod failed.
The performance gain is negligible, most of the time it copies <10 VertexBuffer refs unless I overlook something.

Fixes 2114
@MelVimL
Copy link
Contributor Author

MelVimL commented Oct 16, 2023

Fixes #2114

@stephengold stephengold linked an issue Oct 17, 2023 that may be closed by this pull request
@stephengold stephengold added this to the Future Release milestone Oct 17, 2023
@stephengold stephengold changed the title Fixes 2114 Fixes #2114 (nulls in LodGenerator result) Oct 17, 2023
@stephengold stephengold added Easy first fix bug Something that is supposed to work, but doesn't. More severe than a "defect". and removed Easy first fix labels Oct 17, 2023
@riccardobl riccardobl added the hacktoberfest-accepted PRs that are Hacktoberfest valid (Optional if PR is merged or approved during the month of october) label Oct 17, 2023
Copy link
Member

@riccardobl riccardobl left a comment

Choose a reason for hiding this comment

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

The following unit tests are currently failing in this PR:
com.jme3.tools.LodGeneratorTest > testMonkeyReductionProportional
com.jme3.tools.LodGeneratorTest > testMonkeyReductionConstant
com.jme3.tools.LodGeneratorTest > testMonkeyReductionCollapsCost

This could mean that the tests need to be updated or that the PR introduces issues.

@MelVimL
Copy link
Contributor Author

MelVimL commented Oct 17, 2023

Build test failed because of 3 tests I made. Locale execution without gradle works, with gradle build it fails.
I guess the causing part is this:

URL assetCfgUrl = JmeSystem.getPlatformAssetConfigURL(); AssetManager assetManager = JmeSystem.newAssetManager(assetCfgUrl);

Error: 
F:\Programming\jmonkeyengine\jme3-plugins-json-gson\src\main\java\com\jme3\plugins\gson\GsonObject.java:81: warning: [unchecked] unchecked conversion
    Entry<String, JsonElement>[] entries = new Entry[entrySet.size()];
                                           ^

required: java.util.Map.Entry<java.lang.String,com.jme3.plugins.json.JsonElement>[]
found: java.util.Map.Entry[]
1 warning
\jmonkeyengine\jme3-plugins\src\gltf\java\com\jme3\scene\plugins\gltf\TextureTransformExtensionLoader.java:135: warning: [unchecked] unchecked conversion
Map<Integer, Matrix3f> transformMap = loader.fetchFromCache("textureTransformData", 1, HashMap.class);
^
required: java.util.Map<java.lang.Integer,com.jme3.math.Matrix3f>
found: java.util.HashMap
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 warning

320 tests completed, 3 failed, 1 skipped

FAILURE: Build failed with an exception.

@MelVimL
Copy link
Contributor Author

MelVimL commented Oct 19, 2023

@riccardobl I removed the Test decorator. I guess it is an issue with loading the Jaime.j3o asset while building.

@stephengold
Copy link
Member

@riccardobl Do you support disabling the failing junit tests?

@MelVimL
Copy link
Contributor Author

MelVimL commented Nov 1, 2023

@stephengold @riccardobl I added javadoc and the licenses.

@riccardobl
Copy link
Member

riccardobl commented Nov 8, 2023

Apologies for the late reply, somehow i've missed the notifications.
These tests were added in this pr correct? In that case i have nothing against disabling them if there are technical challenges in making them work.
I think this can be merged

@MelVimL
Copy link
Contributor Author

MelVimL commented Nov 8, 2023

@riccardobl
"These tests were added in this pr correct?":
Yes.

@riccardobl riccardobl merged commit cf74247 into jMonkeyEngine:master Nov 9, 2023
14 checks passed
@stephengold stephengold modified the milestones: Future Release, v3.7.0 Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to work, but doesn't. More severe than a "defect". hacktoberfest-accepted PRs that are Hacktoberfest valid (Optional if PR is merged or approved during the month of october)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LodGenerator: null in returning VertexBuffer.
4 participants