kpovmodeler not working with povray 3.7 #2

Merged
SlavekB merged 1 commits from bug/2765/povray3.7 into master 8 months ago
deloptes commented 4 years ago
Collaborator

A small fix to make it work with povray3.7

https://bugs.trinitydesktop.org/show_bug.cgi?id=2765

A small fix to make it work with povray3.7 https://bugs.trinitydesktop.org/show_bug.cgi?id=2765
Owner

Povray 3.7 does not allow radiosity? Or is it necessary to use other option?

In any case, this patch needs further work.

Povray 3.7 does not allow radiosity? Or is it necessary to use other option? In any case, this patch needs further work.
SlavekB added the
PR/not-ok
label 4 years ago
Poster
Collaborator

AFAIR this option is gone

AFAIR this option is gone
Poster
Collaborator

http://www.povray.org/documentation/3.7.0/r3_2.html#r3_2_8_8

as I mentioned in the original bug a lot needs to be reworked, but the purpose of this patch is to make it at least rendering the model, as otherwise it renders the software mostly useless.

regards

http://www.povray.org/documentation/3.7.0/r3_2.html#r3_2_8_8 as I mentioned in the original bug a lot needs to be reworked, but the purpose of this patch is to make it at least rendering the model, as otherwise it renders the software mostly useless. regards
SlavekB requested changes 8 months ago
SlavekB left a comment
Owner

Because it is no longer necessary to deal with compatibility with older versions of povray, so there is no need to leave the code.

Because it is no longer necessary to deal with compatibility with older versions of povray, so there is no need to leave the code.
// if( m_radiosity )
// cl.append( TQString( "+QR" ) );
// else
// cl.append( TQString( "-QR" ) );
Owner

Instead of leaving the code, it is now appropriate to delete the code, including related GUI options.

Instead of leaving the code, it is now appropriate to delete the code, including related GUI options.
deloptes marked this conversation as resolved
deloptes force-pushed bug/2765/povray3.7 from 8bcebad8ec to 23b8e49047 8 months ago
deloptes requested review from SlavekB 8 months ago
MicheleC approved these changes 8 months ago
MicheleC left a comment
Owner

Looks ok to me. Will wait for final approval from Slavek since he has followed this from the beginning.

Looks ok to me. Will wait for final approval from Slavek since he has followed this from the beginning.
SlavekB requested changes 8 months ago
SlavekB left a comment
Owner

It seems insufficient for me. My idea is to advance and remove the Radiosity checkbox from the GUI and the corresponding code. Now there is in the GUI checkbox that has no effect at all, which seems confusing.

It seems insufficient for me. My idea is to advance and remove the Radiosity checkbox from the GUI and the corresponding code. Now there is in the GUI checkbox that has no effect at all, which seems confusing.
Owner

There is indeed extra code related to radiosity, I also noticed this morning. The key point is to make sure we remove what is no longer required and whether to do it in this PR or as a separate one, while using this PR only to avoid setting unused (unsupported?? - I don't know) parameters anymore.

There is indeed extra code related to radiosity, I also noticed this morning. The key point is to make sure we remove what is no longer required and whether to do it in this PR or as a separate one, while using this PR only to avoid setting unused (unsupported?? - I don't know) parameters anymore.
Poster
Collaborator

Sorry, I never knew there is option for radiosity in the GUI.

Sorry, I never knew there is option for radiosity in the GUI.
Owner

Sorry, I never knew there is option for radiosity in the GUI.

I think the respective checkbox is here: pmrendermodesdialog.cpp:350. At the same time, there will be unnecessary methods setRadiosity(...), radiosity() and variable m_radiosity.

> Sorry, I never knew there is option for radiosity in the GUI. I think the respective checkbox is here: [pmrendermodesdialog.cpp:350](src/branch/master/kpovmodeler/pmrendermodesdialog.cpp#L350). At the same time, there will be unnecessary methods `setRadiosity(...)`, `radiosity()` and variable `m_radiosity`.
Poster
Collaborator

Sorry, I never knew there is option for radiosity in the GUI.

I think the respective checkbox is here: pmrendermodesdialog.cpp:350. At the same time, there will be unnecessary methods setRadiosity(...), radiosity() and variable m_radiosity.

yes, obviously there is more work to do. I will have a look, update the code and request review again.
I'll change this to WIP

> > Sorry, I never knew there is option for radiosity in the GUI. > > I think the respective checkbox is here: [pmrendermodesdialog.cpp:350](src/branch/master/kpovmodeler/pmrendermodesdialog.cpp#L350). At the same time, there will be unnecessary methods `setRadiosity(...)`, `radiosity()` and variable `m_radiosity`. yes, obviously there is more work to do. I will have a look, update the code and request review again. I'll change this to WIP
deloptes added the
PR/wip
label 8 months ago
deloptes added 1 commit 8 months ago
deloptes removed the
PR/wip
PR/not-ok
labels 8 months ago
deloptes requested review from SlavekB 8 months ago
SlavekB reviewed 8 months ago
SlavekB left a comment
Owner

It looks good, I propose one editing regarding modes 9, 10 and 11.
See comment below.

It looks good, I propose one editing regarding modes 9, 10 and 11. See comment below.
I18N_NOOP( "9: Compute media" ),
I18N_NOOP( "10: Compute radiosity but no media" ),
I18N_NOOP( "11: Compute radiosity and media" )
I18N_NOOP( "9: Compute media" )
Owner

According to documentation, modes 10 and 11 remain available, however, they seem to be identical to 9. I propose to do an item as such as 6, 7… => give 9, 10, 11… in one item and update description by documentation. The description could also be updated for item 6, 7….

See https://wiki.povray.org/content/Reference:Tracing_Options#Quality_Settings

According to documentation, modes 10 and 11 remain available, however, they seem to be identical to 9. I propose to do an item as such as *6, 7…* => give *9, 10, 11…* in one item and update description by documentation. The description could also be updated for item *6, 7…*. See https://wiki.povray.org/content/Reference:Tracing_Options#Quality_Settings
Owner

Sounds like a good idea.

Sounds like a good idea.
deloptes added 1 commit 8 months ago
deloptes added 1 commit 8 months ago
Poster
Collaborator

I also think c_qualityToIndex needs to be updated accordingly and I hope the last change does this correctly. So if OK, I will squash the commits.

I also think c_qualityToIndex needs to be updated accordingly and I hope the last change does this correctly. So if OK, I will squash the commits.
deloptes added 1 commit 8 months ago
Poster
Collaborator

I think this last commit is also necessary. I do not understand why it would return numQuality-1

I think this last commit is also necessary. I do not understand why it would return numQuality-1
deloptes requested review from MicheleC 8 months ago
SlavekB approved these changes 8 months ago
SlavekB left a comment
Owner

It looks good. Everything seems to me okay.

It looks good. Everything seems to me okay.
MicheleC requested changes 8 months ago
MicheleC left a comment
Owner

Please check and clarify the comment below.

Please check and clarify the comment below.
index = 0;
if( index >= numQuality )
index = numQuality - 1;
if( index > numQuality )
Owner

Is this change correct? Old code was setting the limit of the index to "numQuality - 1", while the new code is going up to "numQuality" and then using the value to access the array.
If we call the function indexToQuality() with argument value equals to "numQuality" we are going to do an invalid access to the element past the last in the array.
The original code seems correct to me.

Is this change correct? Old code was setting the limit of the index to "numQuality - 1", while the new code is going up to "numQuality" and then using the value to access the array. If we call the function indexToQuality() with argument value equals to "numQuality" we are going to do an invalid access to the element past the last in the array. The original code seems correct to me.
Owner

Yes, you seem to be the truth – numQuality mentions the size, so the index must be one smaller.

Yes, you seem to be the truth – `numQuality` mentions the size, so the `index` must be one smaller.
deloptes marked this conversation as resolved
deloptes force-pushed bug/2765/povray3.7 from 1f1a0cb713 to 72d4668eea 8 months ago
deloptes requested review from MicheleC 8 months ago
Poster
Collaborator

@MicheleC thank you, I have overlooked this.
Now fixed this and squashed into one commit.

@MicheleC thank you, I have overlooked this. Now fixed this and squashed into one commit.
MicheleC approved these changes 8 months ago
MicheleC left a comment
Owner

Looks good now.

Looks good now.
Owner

@MicheleC thank you, I have overlooked this.
Now fixed this and squashed into one commit.

no worries 😄

> @MicheleC thank you, I have overlooked this. > Now fixed this and squashed into one commit. no worries :smile:
SlavekB approved these changes 8 months ago
SlavekB left a comment
Owner

Thanks, now all seems good.

Thanks, now all seems good.
SlavekB merged commit 72d4668eea into master 8 months ago
SlavekB deleted branch bug/2765/povray3.7 8 months ago
SlavekB added this to the R14.0.11 release milestone 8 months ago

Reviewers

MicheleC approved these changes 8 months ago
SlavekB approved these changes 8 months ago
The pull request has been merged as 72d4668eea.
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.