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

Wrong instance count in subsubmodels with CONSOLIDATE_INSTANCE_COUNT AT_TOP #537

Closed
sponce opened this issue Apr 21, 2021 · 10 comments
Closed
Assignees
Milestone

Comments

@sponce
Copy link

sponce commented Apr 21, 2021

Subject

When having a complex setup where a subsubmodle is included in different submodels and these submodels are inserted several times in the main model, the instance count for the subsubmodel is wrong.

Environment

  • Version of LPub3D : dev-release 2.4.2 revision 40 build 2472 hash e7c39cd
  • Operating system : linux debian,x86_64

Screenshots

  • Configuration->Preferences->General
  • Configuration->Preferences->Rendering
  • Configuration->Preferences->Publishing
  • Configuration->Preferences->Logging
  • Configuration->Preferences->Other

Steps to reproduce

Just open the attached test.mpd file. First page shows a count of 3 when it should be 4.
image

Expected behaviour

Count on first page should be 4.

Actual behaviour

Count is 3, seems to be that it forgets that test2 is included twice at the top level, as including it only once still gives 3, which is correct in this case.
In other words, when having subsubmodels, it looks like the number of top level inclusions are not counted and multiplied by the number of lower level ones.

Workaround

Simply force the number by hand via "0 !LPUB PAGE SUBMODEL_INSTANCE_COUNT_OVERRIDE 4"

Solution suggestion

I can try to fix it in the code myself. Did not have time to look at it yet.

@trevorsandy
Copy link
Owner

Thank you for reporting this behaviour.

Indeed, there may be instance count edge cases that are not calculated as there are so many combinations of submodel inclusions possible. There are even cases where the correct number of modelled instances has to be overridden to reflect the actual physical build requirements.

Untreated cases are precisely the reason for the count override command 0 !LPUB PAGE SUBMODEL_INSTANCE_COUNT_OVERRIDE <number>

However, my approach is to add edge cases, if reasonable, as they are reported so I will take a look.

Please provide the steps to reproduce or an example model file that reproduces this behaviour.

Just open the attached test.mpd file. First page shows a count of 3 when it should be 4.

There is no attachment.

Cheers,

@sponce
Copy link
Author

sponce commented Apr 21, 2021

Damn I did not notice the file did not attach because .mpd is unknown extension. Sorry. Here it is now.

test.mpd.txt

@sponce
Copy link
Author

sponce commented Apr 21, 2021

By the way, I do not think this is particularly urgent and I'm happy to look at it when I manage to find some time.

@trevorsandy
Copy link
Owner

By the way, I do not think this is particularly urgent and I'm happy to look at it when I manage to find some time.

Very well.

Cheers,

@trevorsandy
Copy link
Owner

The case presented in your example model file is not an edge case, the current LPub3D countInstances behaviour should produce 4 instances of test3.ldr.

I’ll take a look.

Cheers,

@trevorsandy
Copy link
Owner

trevorsandy commented Apr 22, 2021

I found the reason for this behaviour.

When a submodel instance is encountered for the first time, I set a flag stating it has been counted, when the submodel is encountered again, its instance count is automatically incremented and the loop continues on to the next line. This approach optimizes performance by avoiding having to parse the same submodel every time it is encountered. However, if that submodel includes a submodel (as in your case), the included submodel instance will not be counted.

Although this is not an edge case, enabling it in the current instance count call will have a significant impact on performance. If I can come up with a routine that preserves performance and captures included submodel instances on subsequent encounters, I'll update the count instance call. Otherwise, this case will simply have to use the override command. Performance is important for the count instance call as it is triggered at every page transition.

Cheers,

@sponce
Copy link
Author

sponce commented Apr 22, 2021

Thanks for the quick analysis. In case we stay with the override command, I would then suggest a small modification of a few lines, namely :

  • displayInstanceCount = countInstances && instances > 1;
  • displayInstanceCount = steps->meta.LPub.subModel.showInstanceCount.value() && instances > 1;
  • displayInstanceCount = countInstances && instances > 1;

    The thing is that due to the instances > 1 part, override has no effect if the initial count was 1, and there is then no known work-around to the problem in such cases. Probably a simple and || override is set` would be sufficient.

@trevorsandy
Copy link
Owner

trevorsandy commented Apr 22, 2021

I'll add your suggestion as if the override value is > 1. I also decided to update the countInstance call with the following:

  1. Add _subFileIndexes type (Vector<int>) to LDrawSubFile class
  2. If a SubFile contains a SubFile, add the child SubFile index to the parent _subFileIndexes :
const int subFileIndex = getSubmodelIndex(tokens[14]);
if (!f->_subFileIndexes.contains(subFileIndex))
    f->_subFileIndexes.append(subFileIndex);
  1. When the parent SubFile is automatically incremented, also check for children:
for (int i = 0; i < f->_subFileIndexes.size(); i++)  {
    QMap<QString, LDrawSubFile>::iterator s = _subFiles.find(getSubmodelName(f->_subFileIndexes.at(i)));
    if (s != _subFiles.end())
        ++s->_instances;
}

Result from your example file:

Screenshot - 22_04_2021 , 09_12_02

Before update count elapsed time: 0.039 second
After update count elapsed time: 0.050 second

Cheers,

@sponce
Copy link
Author

sponce commented Apr 22, 2021

Excellent ! thanks a lot for the fast fix.

@trevorsandy trevorsandy added this to the 2.4.3 milestone Apr 22, 2021
@trevorsandy
Copy link
Owner

trevorsandy commented Apr 22, 2021

I forgot to mention. This is all you need for your example file to be correct.

0 FILE test.ldr
0 Name: test.ldr
0 !LPUB PAGE SIZE GLOBAL 11.75 8.25
0 !LPUB CONSOLIDATE_INSTANCE_COUNT GLOBAL AT_TOP
1 0 -40 0 100 1 0 0 0 1 0 0 0 1 test2.ldr
1 0  40 0 100 1 0 0 0 1 0 0 0 1 test2.ldr
1 0   0 0  50 1 0 0 0 1 0 0 0 1 test1.ldr
0 STEP
0 NOFILE
0 FILE test2.ldr
0 Name: test2.ldr
1 0 0  0 0 1 0 0 0 1 0 0 0 1 test3.ldr
1 2 0 24 0 1 0 0 0 1 0 0 0 1 3001.dat
0 NOFILE
0 FILE test1.ldr
0 Name: test1.ldr
1 0 0   0 -10 1 0 0 0 1 0  0 0 1 test3.ldr
1 0 0   0 110 1 0 0 0 1 0  0 0 1 test3.ldr
1 0 0 -32  50 0 0 1 0 1 0 -1 0 0 3034.dat
0 NOFILE
0 FILE test3.ldr
0 Name: test3.ldr
1  1 0   0 0 1 0 0 0 1 0 0 0 1 3001.dat
1 71 0 -24 0 1 0 0 0 1 0 0 0 1 3001.dat
0 NOFILE

Cheers,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants