Skip to content

Conversation

@kkolyan
Copy link
Contributor

@kkolyan kkolyan commented May 4, 2020

What've done

OBJ model format has markup element to organize objects in groups (without nesting). Currently, this markup is ignored by jME's OBJLoader - only shape and material is loaded. This PR implements loading of groups as dedicated children of resulting Spatial.

Justification

In current verison there is no legitimate way of transfering any "tags" from model editor to game code with OBJ format models. This feature is vital for loading interactive models from OBJ format. Though OBJ format may be too primitive for serious modern character models, it's easy to support, it's tools-friendly, it's human-friendly, it's still supported by many editors and its capabilities are enough for static geometry modeling such as game areas for non-high-end games.

Forum discussion

https://hub.jmonkeyengine.org/t/objloader-enhancement-named-groups-support/43243

@8Keep
Copy link
Contributor

8Keep commented May 4, 2020

Can you write a test for this? Or a sample for usage.

@kkolyan kkolyan force-pushed the objloader_groups branch from 7775577 to 8703b72 Compare May 4, 2020 14:37
@kkolyan
Copy link
Contributor Author

kkolyan commented May 4, 2020

Can you write a test for this?

Sure:) Test added

Or a sample for usage.

where to place it? Or test is enough?

@stephengold
Copy link
Member

Thanks for the testcase. Please move it from jme3-core to to jme3-examples/src/main/java/jme3test/model

@kkolyan
Copy link
Contributor Author

kkolyan commented May 8, 2020

what if I leave it here and create more demonstrative example in jme3test?

@stephengold
Copy link
Member

OK.

@kkolyan
Copy link
Contributor Author

kkolyan commented May 8, 2020

Is it ok to move test model and material to jme3-testdata and add it as test dependency to jme3-core?
I mean to add

dependencies {
    testCompile project(':jme3-testdata')
}

to jm3-core/build.gradle.

Looks doubtfull, because as I see, you keep jme3-core build.gradle almost empty.

@stephengold
Copy link
Member

I think it would be OK as a test dependency.

@kkolyan kkolyan force-pushed the objloader_groups branch 2 times, most recently from 7653663 to 3f5d840 Compare May 8, 2020 23:59
@kkolyan
Copy link
Contributor Author

kkolyan commented May 9, 2020

looks like checks failed due to some infrastructural problems

@stephengold
Copy link
Member

I'll re-run those jobs.

@kkolyan
Copy link
Contributor Author

kkolyan commented May 9, 2020

It stills breaks. Looks like I handle text in non-portable way. Will fix later.

@kkolyan
Copy link
Contributor Author

kkolyan commented May 9, 2020

Is there some quota for check runs? This issue is Mac specific and I do not have Mac, so it may be necessary to run checks here many times. Is it ok?

@stephengold
Copy link
Member

GitHub Actions is free for public repositories. As long as you're not intentionally abusing the servers, there shouldn't be any issues. https://help.github.com/en/github/setting-up-and-managing-billing-and-payments-on-github/about-billing-for-github-actions

@kkolyan kkolyan force-pushed the objloader_groups branch 3 times, most recently from 4858ab4 to cb73f1e Compare May 10, 2020 20:38
@kkolyan
Copy link
Contributor Author

kkolyan commented May 10, 2020

comment in example updated

@stephengold
Copy link
Member

I'll take a closer look today and hopefully integrate this PR.

@stephengold
Copy link
Member

I looked, and I mostly liked what I saw. Apologies for the slowness of our PR process.

I like the new warnings for OBJ assets that contain merge groups or smoothing groups!

TestObjGroupsLoading is a clever test app ... though I wish it had an indicator in the center of the display to help the user aim the camera. A BitmapText containing "+" (as seen in TestBrickTower and many other examples) would be perfect. But that's a mere quibble that shouldn't prevent integration.

The main issue with this PR is simply that the 2 new source files ("ObjLoaderTest.java" and "TestObjGroupsLoading.java") lack licenses. Given the nuisance that issue #1001 has caused, I won't integrate those files without compatible licenses.

Please license these files, using similar source files are models.

@kkolyan kkolyan force-pushed the objloader_groups branch from cb73f1e to beb168a Compare May 16, 2020 09:32
@kkolyan
Copy link
Contributor Author

kkolyan commented May 16, 2020

crosshair and licences added.

@kkolyan
Copy link
Contributor Author

kkolyan commented May 16, 2020

sorry for non-convenient manner of ammend-force-push every commit. At work we never do so (we are ok with inflated history). But for jME I thought it's better to apply such little change as single squashed commit. Now I think it would be better to squash it only right before integration.

@stephengold
Copy link
Member

I, too, would prefer to squash during integration, rather than having forced pushes. I'll take another look.

@stephengold stephengold merged commit 0f06c14 into jMonkeyEngine:master May 16, 2020
@stephengold
Copy link
Member

Looks good. Thanks for your contribution to JMonkeyEngine!

@kkolyan
Copy link
Contributor Author

kkolyan commented May 17, 2020

Looks good. Thanks for your contribution to JMonkeyEngine!

I was glad to participate!

@stephengold stephengold added this to the v3.4.0 milestone Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants