Open sl-service-account opened 9 years ago
Kyle Linden commented at 2015-03-13T02:05:10Z
Hi Drongle,
It appears your model names are wrong with the new mesh import feature set. Please review this newly published mesh import test document and let us know if you find anything else.
http://wiki.secondlife.com/wiki/Mesh_Import_test
Thanks!
Drongle McMahon commented at 2015-03-19T02:37:53Z, updated at 2015-03-19T02:51:35Z
Kyle. It is true that the single meshes in each file have different names. However, I do not see where in the test document any behaviour is defined for that situation. neither am I aware of anywhere (other than, perhaps the source code) where any "correct" naming scheme is defined. Could you perhaps tell me what is required for "correct" names, or point me to where it is documented?
Furthermore, in llFloaterModelPreview from the Project-Importer repository, there are comments that seem to indicate the intention to fall back to the old index-based LOD model associations...
1396 // If you don't take the time to name your LOD and PHYS meshes with the name of their corresponding mesh in the HIGH LOD, then the indexed method will be attempted below.
1423 & 1479 // Fall back to old method of index-based association if we could not find a match based on the mesh names
Are you now saying that there is compulsory naming scheme, so that there is no fallback, and huge numbers of existing collada files will be rendered useless?
Also, the release notes for the viewer version in question say...
"LODs and Physics reps can now be explicitly associated with a given mesh in the full LOD model using name-based matching."
This says "can" not "must". If it's to be the latter, it needs to be changed.
"By properly naming the meshes in your lower LOD meshes and physics reps, you can avoid issues with ordering of the meshes within your DCC tool and other material mismatching errors."
Where is "properly naming" defined? Other than the source code ... is it perhaps the suffixes "_LOD2", "_LOD1", "_LOD0", "LOD_PHYS"?* If so, that is overly restrictive. For example, any suffix should do, since the LOD is defined by the uploader slot the file is selected into and does not therefore have to be specified by the suffix. Perhaps best would be a minimum length unique match, so that a suffix on the high LOD would not prevent matching.
In any case, if there are problems with the names I used, it remains the case that the observed behaviour is unacceptable. There is no possible rational argument for substituting the effect of "Use LOD above" for the obvious intention of using something from the chosen file. Instead, the choice should revert to its previous state and an informative error message should be generated. Telling the user to look in the log file may be acceptable for debugging, but surely not for normal use.
*I guess I will have to try this when I have a chance.
ETA: The first of the quoted source code comments would seem to indicate that the corresponding meshes in each LOD have to have the same name. If that is the case, that is a major inconvenience - for example, if you keep all your LOD models in the same Blender file, which is the most efficient way to make them, they cannot have the same names. So you are forced to rename them every time you export them, then rename them back again after exporting.
Drongle McMahon commented at 2015-03-19T03:40:41Z
Tested (1) after renaming all files so that the mesh names (and IDs) were the same in all of them. (2) after adding the suffixes from the source code, _LOD2, _LOD1, _LOD0 to names & IDs in medium, low and lowest LOD files respectively. This had no effect on the unexpected behaviour originally reported. So neither of those naming schemes appears to be "correct".
Kyle Linden commented at 2015-03-19T04:21:18Z
Hi Drongle,
Thank you for reviewing the new import method, your feedback is appreciated. The naming scheme is not clearly documented and we will need to address that before we move this viewer beyond the project stage. Regarding the naming scheme, here is what I did to make your models upload and behave as you expected.
File names remained the same.
The 'fallback' method does work but is now much more strict about model and material relationships.
Please let me know if you have any other feedback.
Thank you!
Drongle McMahon commented at 2015-03-19T08:57:05Z, updated at 2015-03-19T09:07:58Z
Kyle. Thank you for your prompt reply.
Unfortunately that doesn't work when I do it. Before my previous comment, I had tested renaming them by editing the collada file, as the comment indicated. I have now done the same renaming them in Blender, as you instructed. It still didn't work properly. In the Blender exporter, the name and ID are not the same, and I don't know which the viewer prefers to use. Also I don't know where it takes the name from.
Blender names the
I will upload the files here. They are called mcylXrenb.dae for the unaltered Blender exports, and mcylXrenbx for those edited to remove the suffix, where X is the number of cylinder sides. It might be helpful if you could upload the files you obtained after renaming, so that we can inspect them for differences. May I also be so bold as to ask whether you did check the triangle and vertex counts in the uploader during the upload, and/or whether you inspected the actual uploaded LOD meshes (e.g. by dialing in low values of renderVolumeLODFactor with a prim cylinder beside the mesh to check the switch). Simply completing an upload without those checks does not test for the unexpected behaviour, as the upload proceeds to completion, but with the wrong model in each slot.
Drongle McMahon commented at 2015-03-19T20:22:30Z
Here is an extract of the log file obtained with ImporterDebug on when uploading the LODs mcylXrenb.dae, putting them in order into slots high, medium,low, lowest. These have the specified naming scheme, "cylinder" in the high LOD (X=6) and "cylinder_LOD1" for the low LOD (LOD1) etc., for names and IDs of
Notes:
Uploading mcyl6renbx.dae, with mcyl5renbx.dae, mcyl4renbx.dae, mcyl3renbx.dae in medium, low and lowest slots.
This is after specifying the low LOD (LOD1) file.
Line 04: the names and IDs of
01 2015-03-19T19:09:39Z INFO: LLDAELoader::OpenFile: Collada Importer Version: 1.4.1
02 2015-03-19T19:09:39 Z INFO: LLDAELoader::OpenFile: Dae version 1.4.1
03 2015-03-19T19:09:39Z INFO: LLDAELoader::OpenFile: Importing cylinder_LOD1_0
04 2015-03-19T19:09:39Z INFO: LLDAELoader::OpenFile: cylinder_LOD1_0 references mat-material
05 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Verts: 6
06 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Tris: 2
07 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Faces: 1
08 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Material mat-material
09 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD1 LOD 1 Verts: 6
10 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD1 LOD 1 Tris: 2
11 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD1 LOD 1 Faces: 1
12 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD1 LOD 1 Material mat-material
13 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Verts: 38
14 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Tris: 24
15 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Faces: 1
16 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Material mat-material
17 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Verts: 38
18 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Tris: 24
19 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Faces: 1
20 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Material mat-material
21 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: Falling back to model index 0 for LOD 4 of cylinder_1
22 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: List of models for LOD 4 did not include index 0
23 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: Assigning LOD3 for cylinder_1 to found match cylinder_1
24 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: Falling back LOD2 for cylinder_1 to found cylinder_1
26 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: Falling back LOD1 for cylinder_1 to found cylinder_1
27 2015-03-19T19:09:39Z INFO: LLModelPreview::rebuildUploadData: Assigning LOD0 for cylinder_1 to found match cylinder_1_LOD0
28 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Verts: 6
29 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Tris: 2
30 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Faces: 1
31 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Material mat-material
32 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Verts: 38
33 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Tris: 24
34 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Faces: 1
35 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Material mat-material
36 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Verts: 38
37 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Tris: 24
38 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Faces: 1
39 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Material mat-material
40 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Verts: 38
41 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Tris: 24
42 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Faces: 1
43 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Material mat-material
44 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Verts: 6
45 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Tris: 2
46 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Faces: 1
47 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1_LOD0 LOD 0 Material mat-material
48 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Verts: 38
49 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Tris: 24
50 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Faces: 1
51 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 1 Material mat-material
52 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Verts: 38
53 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Tris: 24
54 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Faces: 1
55 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 2 Material mat-material
56 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Verts: 38
57 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Tris: 24
58 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Faces: 1
59 2015-03-19T19:09:39Z INFO: LLModelPreview::updateStatusMessages: Instance cylinder_1 LOD 3 Material mat-material
Kyle Linden commented at 2015-03-20T03:44:01Z
Hi Drongle,
I will need to review your newly uploaded models and see if I can import them myself. In theory you did the right things but it still failed. This has me puzzled and I'll have to investigate further.
Thank you,
Drongle McMahon commented at 2015-03-22T11:34:24Z, updated at 2015-03-24T11:52:14Z
I think I may have found where these problems are coming from...
LLDAELoader::OpenFile and LLFloaterModelPreview::rebuildUploadData both use LLDAELoader::getElementLabel
to find the name of each object being uploaded in a file. OpenFile starts looking in the
GetElementLabel tries first to use the name attribute, and then the ID attribute of the tag. If it
doesn't find either, then it looks in the parent tag. The
When it takes the name or ID from the parent tag, getElementLabel adds an index to the name to distinguish sibling children of parents with multiple relevant child tags. This is added as a string "_X", where "X" is the zero-based index. If the name from the attribute has one of the pre-defined LOD suffixes, containing the substring "_LOD", then it is supposed to insert the added index immediately in front of that substring. Otherwise, it appends it to the end of the attribute name.
OpenFile puts a messgae in the log giving its version of each object name it is importing. It does this with
the names derived from the
Drongle McMahon commented at 2015-03-23T19:29:15Z, updated at 2015-03-25T20:04:14Z
Another thought - getElementLabel could look at the url attribute of
Furthermore, if it used the url (-#) as the preferred option, and used the ID as the second option, then I think it would necessarily get the same name for the calls from OpenFile and from rebuildUploadData, because the url in
Kyle Linden commented at 2015-03-27T16:47:29Z
Hi Drongle,
Thank you for the updated information and your investigation results. It was helpful and appreciated.
Thanks!
Kyle Linden commented at 2015-03-27T22:51:06Z
Hi Drongle,
It turns out this is already fixed! Will you please confirm this is working as expected with the following build.
Thank you
Drongle McMahon commented at 2015-03-29T02:15:12Z
I would say no, it is not fixed. In one respect it is, because the sibling index is now inserted in the right place. That does let it work as intended in the case that the "correct" naming convention is used.
However, if it is not used, (a) the result is not the intended fallback to the old index-based method. Instead, (b) the uploader gives an incorrect error message "Material of model is not a subset of reference model", when in the test case two files do use the same material (and material name). This blocks upload by disabling the calculate button. (c) the log file gives a different message, saying the offending model was not used, which is true. (d) Instead of restoring the previous state, it still uses the next higher LOD instead, which is clearly not the intended result, and not useful because the upload is blocked anyway.
This still leaves all existing LOD files unusable (except where anyone just happens ton have used the new naming convention that they could not have anticipated) because the fallback to indexed method does not happen. I would anticipate that users would find that unacceptable.
The test files I used are qcyl6.dae (high LOD), qcyl5_lod2uc.dae (medium LOD consistent with new naming convention) and qcyl5.dae (medium LOD, not consistent).
By the way, the "LOD2" etc appear to be case sensitive. At the very least, please make it case insensitive. Preferably, make it accept other conventions, such as "_me*", "_lo$" etc etc., or best of all, use a fuzzy or partial matching algorithm and accept it whenever it's non-ambiguous.
In the case of files that only have one object (about 99% of my files), why impose anything at all? There cannot be any ambiguity in those cases. So insisting on a naming convention there, and disabling so many existing files, is then a completely unnecessary burden. Skipping the tests when the object count is 1 should be trivial to code. (Of course it wouldn't be necessary if the fallback to indexed method worked). More difficult when objects are split by > 8 materials, but that's a new thing anyway, so that no working legacy files would be affected.
Kyle Linden commented at 2015-04-07T17:15:48Z
Hi Drongle,
Thank you for the feedback. I have shared this with the team.
Keep an eye out for a new Project Viewer soon.
Thanks again for your efforts.
Steps to Reproduce
Files m1cylx, where x=6,5,4,3 are cylinders with x sides.
Actual Behavior
Each lower LOD slot gets filled with the LOD above, whatever file is loaded in the file selector.
Expected Behavior
Expected specified meshes to be uploaded for all LODs. Expected uploader triangle counts to be correct for the specified files.
Other information
This blocks very necessary tests of effect of model fragmentation with explicit LOD meshes. Needs immediate repair because it blocks other tests.
Attachments
Links
Related
Original Jira Fields
| Field | Value | | ------------- | ------------- | | Issue | BUG-8734 | | Summary | [Project-Importer] LoadFromFile in lower LOD slots fails. | | Type | Bug | | Priority | Unset | | Status | Been Triaged | | Resolution | Triaged | | Reporter | Drongle McMahon (drongle.mcmahon) | | Created at | 2015-03-10T15:33:25Z | | Updated at | 2015-04-09T20:39:30Z | ``` { 'Business Unit': ['Platform'], 'Date of First Response': '2015-03-12T21:05:10.066-0500', "Is there anything you'd like to add?": 'This blocks very necessary tests of effect of model fragmentation with explicit LOD meshes. Needs immediate repair because it blocks other tests.', 'Severity': 'Unset', 'System': 'SL Viewer', 'Target Viewer Version': 'viewer-development', 'What just happened?': 'Each lower LOD slot gets filled with the LOD above, whatever file is loaded in the file selector.\r\n1. All LODs end up as the high LOD, m1cyl6.\r\n2. Lowest LOD becomes same as low LOD.\r\n3. Low LOD becomes same as medium LOD.', 'What were you doing when it happened?': 'Files m1cylx, where x=6,5,4,3 are cylinders with x sides.\r\n1. Upload model m1cyl6.dae; put m1cyl5, m1cyl4, m1cyl3 in sequential LOD slots.\r\n2. Upload model m1cyl6.dae; put m1cyl3 in lowest LOD slot\r\n3. Upload model m1cyl6.dae; put m1cyl3 in low LOD slot\r\n', 'What were you expecting to happen instead?': 'Expected specified meshes to be uploaded for all LODs. Expected uploader triangle counts to be correct for the specified files.', 'Where': 'See env', } ```