kpovmodeler not working with povray 3.7 #2

已合併
SlavekB 3 年之前 將 1 次代碼提交從 bug/2765/povray3.7 合併至 master
協同者

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
所有者

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 6 年之前
發佈者
協同者

AFAIR this option is gone

AFAIR this option is gone
發佈者
協同者

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 3 年之前
SlavekB left a comment
所有者

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" ) );
所有者

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 3 年之前
deloptes requested review from SlavekB 3 年之前
MicheleC approved these changes 3 年之前
MicheleC left a comment
所有者

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 3 年之前
SlavekB left a comment
所有者

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.
所有者

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.
發佈者
協同者

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

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

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`.
發佈者
協同者

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 3 年之前
deloptes added 1 commit 3 年之前
deloptes removed the PR/wip PR/not-ok label 3 年之前
deloptes requested review from SlavekB 3 年之前
SlavekB reviewed 3 年之前
SlavekB left a comment
所有者

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" )
所有者

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
所有者

Sounds like a good idea.

Sounds like a good idea.
deloptes added 1 commit 3 年之前
deloptes added 1 commit 3 年之前
發佈者
協同者

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 3 年之前
發佈者
協同者

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 3 年之前
SlavekB approved these changes 3 年之前
SlavekB left a comment
所有者

It looks good. Everything seems to me okay.

It looks good. Everything seems to me okay.
MicheleC requested changes 3 年之前
MicheleC left a comment
所有者

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 )
所有者

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.
所有者

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 3 年之前
deloptes requested review from MicheleC 3 年之前
發佈者
協同者

@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 3 年之前
MicheleC left a comment
所有者

Looks good now.

Looks good now.
所有者

@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 3 年之前
SlavekB left a comment
所有者

Thanks, now all seems good.

Thanks, now all seems good.
SlavekB merged commit 72d4668eea into master 3 年之前
SlavekB 刪除分支 bug/2765/povray3.7 3 年之前
SlavekB 新增至R14.0.11 release 里程碑 3 年之前

Reviewers

MicheleC approved these changes 3 年之前
SlavekB approved these changes 3 年之前
The pull request has been merged as 72d4668eea.
登入 才能加入這對話。
未選擇里程碑
No Assignees
3 參與者
訊息
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdegraphics#2
Loading…
尚未有任何內容