LDR Importer searches wrong pathes for referenced files #40
Comments
|
sorry, but even when inlining these 2 files I cannot load my model successfully. I have attached the files for you to this issue. I tried to load file "6372.ldr". It references files "6372-Minifig-1.ldr" and "6372-Minifig-2.ldr", which both reference "6372-Chair.ldr". Note that the file references some unofficial parts, so you need to download the latest bunch Please see in the attached screenshot how distorted my import result is :( |
|
|
|
Unfortunately I cannot attach files to this issue. === FILE: 6372.ldr =================================================================== === FILE: 6372-Minifig-1.ldr ====================================================== === FILE: 6372-Minifig-2.ldr ========================================================= === FILE: 6372-Chair.ldr ==================================================== |
|
behaviour confirmed by testing. |
|
I've looked into the sourcecode, and the error is located in function and can be easily spotted there. the pathes being checked are wrong. they currently are: I'll simplify things here for display. I will use C:\LDRAW instead of LDrawDir for simplification. c:\ldraw This order is all wrong in many ways! The proper order would be: (current folder of the currently loaded model) Note that this is a totally different list! The folders c:\ldraw\ and c:\ldraw\unofficial\ need not to be scanned at all. |
|
here is the corrected code. note that you still need to work on the line marked # XXX def locate(pattern): |
|
I could probably fix this very quickly, but as usual, I have very little time today. Will look into it. Thank you for your report and contribution! |
|
I'll comment on all this when I can. Thanks for looking into this! I would not have known how to fix it! If you'd like, fork the repo, make a new branch of the |
|
may I ask to get this fix into the planned release 1.2 It is quite important to have a corrected files search algorithm in it. |
|
If you can write a patch, we can add it to that. We were planning on making v1.2 a rewrite (which is badly needed), but considering the reports and patches added recently, we might end up doing a minor release to tide things over and fix these issues. |
|
you can see the patch above. the only thing missing in it is the line marked # XXX where I have Here is a screenshot with the result Blender gives when using my code suggestion above. |
|
The hardcoded path fix shouldn't be hard. I'll look into it. Looking at your repositories, are you a .NET/Mono programmer? </offtopic> |
|
yes, mainly C++ and C#. For >10 years ;) |
|
whistles |
|
Looks like the only thing missing in the patch is the new /p/8 folder, which I would assume we would need to support. |
|
No biggie, but will we need an option for 'low-res bricks'? |
|
It is not necessary right now. The new value of that option would be "substitute low-res (8ed) primitives". Note that you need to do that both times, i.e., in the official and unofficial folders. So the code above becomes: pathes = [] Adding that option is not very important, as usually a user will most probably not want |
|
Looking at the code, the recursive form of the LDrawFile class strikes again! It's hard to get the proper working directory for the file when the same code is being used to parse the individual parts themselves. Can I use a global variable? |
|
I think that would be the easiest way. While you are at that, I suggest that you optimize the code a little more: So this will solve 2 problems with 1 change :D |
|
And that, my friend, will go with #35, AKA rewrite. :) |
|
yes, but the change suggested above can also be added easily to the current code and would already I would suggest to add the simple fix above to the current implementation Otherwise, I have to wait for this fix until you've finished the refacturing :( , |
|
Is the patch in #40 (comment) the entire |
|
yes. however, adding the global variable mentioned will require to touch more/other locations. |
|
Also, is |
|
currently, I hardcoded isPart to be always true because I didn't want to refacture too much. This requires looking INTO the file to determine if it is a part or not. I did not check where that IsPart flag is being used for. I can only imagine 1 useful use of that information: However, adding that parsing requires a major reworking of the script, so my suggestion is |
|
I seem to have gotten it working correctly on linux, complete with the directory of the current file, but MinnieTheMoocher's ldr file can't find the part: 3062B.DAT Or STUD4.DAT Also, it's spelled 'paths' not 'pathes'. It's a common spelling mistake. :3 EDIT: Added .lower() to fname2 so that it doesn't fail when finding badly capitalized files (see above). All ldraw parts should be named. The bad news is, that means all external files must be lowercase.... Not sure how to fix that. ATM EDIT2: MinnieTheMoocher's solution works, but seems to cause a MAJOR speed hit. Not sure why, it might be something else in the new version that I just forked. My 4kb test file has been going for nearly 30 minutes, removing vertices for about 27 of them, even though I selected "Original LDraw model" |
|
(a) the ldraw files in the file system can be upper or lowercase or mixed. you need to find them case-insensitively... (b) speed: the paths[] array construction should be moved out of the locate function. it is expensive and only needs to be done once. |
|
(a) Just fixed that. (b) The paths array construction slows it down a bit, but the slowest part seems to be the "removing of vertices" |
|
For information, I added the code from here in the What you said here, Steffen, if I can comprehend it enough (bit unfocused ATM), I may attempt to work on it.
Oh yes, the script has taken a major speed hit, but I'm not sure where. His code did slow it down, but it also seems to be slower without it. The only things it could be is #45 or #47, but I'm not sure either of those slow down the actual importing process. The latter should actually speed it up, but I'm not sure. His patch for the proper locations also seems to do something which should not happen. Changing As Steffen (MinnieTheMoocher) said, |
|
Ah, that explains it. Alrighty, I have a complete directory patch. (Linux) EDIT: Apparently the issues with certain parts not importing correctly (mainly concave round parts) has been fixed in git! :D |
gh-40 - "(b) speed: the paths[] array construction should be moved out of the locate function. it is expensive and only needs to be done once."
ghost
assigned
|
I'm closing this issue now because it was fixed with #50, but I'll keep this in mind for later reference for a few things. ;) |
When creating a .ldr file, it sometimes references other *.ldr files
lying next to it in the same folder.
This is seemingly not supported in LDR Importer 1.1:
It fails importing my 6372.dat file which references 6372-Minifig-2.ldr
which is lying next to it.
This should not happen. LDR Importer needs to check folders for files
in the following order:
(1) first see if the referenced file is lying next to the current one
(2) if not found there, check the ldraw\models folder
(3) if not found there, check the ldraw\parts folder
(4) if not found there, check the ldraw\p folder
Step (1) is seemingly missing from the LDR Importer implementation.
I guess that Step (2) also is missing.
As a workaround I currently inline all referenced files into my .ldr file,
but that's something I do not really want to be forced to.
The text was updated successfully, but these errors were encountered: