Refactors models and materials #2
ClosedPublic
Actions

Authored by vladislavbelov on Sep 14 2023, 7:21 PM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP27846: Refactors models and materials, part 2, replaces raw pointer by unique_ptr and…

Summary

Refs D5108.

Continues refactoring and cleaning.

Test Plan

Apply the patch and compile the game
Check that all tests passed

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Sep 14 2023, 7:21 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7287/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Debug:
    14>e:\jenkins\workspace\vs2015-differential\source\renderer\scene.cpp(38): error C2664: 'void SceneCollector::SubmitRecursive(CModelAbstract *)': cannot convert argument 1 from 'const std::unique_ptr<CModelAbstract,std::default_delete<_Ty>>' to 'CModelAbstract *' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    15>e:\jenkins\workspace\vs2015-differential\source\tools\atlas\gameinterface\actorviewer.cpp(260): error C2664: 'void ActorViewerImpl::UpdatePropListRecursive(CModelAbstract *)': cannot convert argument 1 from 'std::unique_ptr<CModelAbstract,std::default_delete<_Ty>>' to 'CModelAbstract *' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\atlas.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/8376/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-gcc7.txt
../../../source/renderer/Scene.cpp: In member function 'virtual void SceneCollector::SubmitRecursive(CModelAbstract*)':
../../../source/renderer/Scene.cpp:38:37: error: no matching function for call to 'SceneCollector::SubmitRecursive(const std::unique_ptr<CModelAbstract>&)'
     SubmitRecursive(props[i].m_Model);
                                     ^
../../../source/renderer/Scene.cpp:28:6: note: candidate: virtual void SceneCollector::SubmitRecursive(CModelAbstract*)
 void SceneCollector::SubmitRecursive(CModelAbstract* model)
      ^~~~~~~~~~~~~~
../../../source/renderer/Scene.cpp:28:6: note:   no known conversion for argument 1 from 'const std::unique_ptr<CModelAbstract>' to 'CModelAbstract*'
make[1]: *** [graphics.make:472: obj/graphics_Debug/Scene.o] Error 1
make: *** [Makefile:127: graphics] Error 2

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/8958/display/redirect

Harbormaster completed remote builds in B22336: Diff 22259.Sep 14 2023, 7:28 PM

vladislavbelov requested review of this revision.Sep 14 2023, 7:28 PM

phosit added a subscriber: phosit.Sep 14 2023, 8:03 PM

phosit added inline comments.

source/graphics/ModelDummy.h
38–41 ↗	(On Diff #22259)	If you want to remove the semicolon on untouched lines do it also for `ValidatePosition` and `InvalidatePosition`.
source/graphics/ObjectEntry.cpp
254–255 ↗	(On Diff #22259)	Can't the `SetAnimation` be called before the `AddProp` and `AddAmmoProp`?
source/graphics/tests/test_Model.h
35–38 ↗	(On Diff #22259)	`std::any_of`

vladislavbelov marked an inline comment as done.Sep 15 2023, 8:02 PM

vladislavbelov added inline comments.

source/graphics/ObjectEntry.cpp
254–255 ↗	(On Diff #22259)	I've checked and it seems it might be called before. BTW the code order is appeared in rP306.
source/graphics/tests/test_Model.h
35–38 ↗	(On Diff #22259)	What I don't like in `std::any_of` is that it seems less readable currently: const auto& map = defines.GetMap(); std::any_of(map.begin(), map.end(), [&define](const auto& pair) { return pair.first == define && pair.second == str_1; });

phosit added inline comments.Sep 15 2023, 10:37 PM

source/graphics/tests/test_Model.h
35–38 ↗	(On Diff #22259)	What do you think about `std::find`? const auto& map = defines.GetMap(); return std::find(map.begin(), bap.end(), std::make_tuple(std::ref(define), str_i)) != map.end();

vladislavbelov added inline comments.Sep 20 2023, 6:20 PM

source/graphics/tests/test_Model.h
35–38 ↗	(On Diff #22259)	Seems better. I'd like to have something like: return std::find(map, {define, str_1}) != map.end();

vladislavbelov added inline comments.Sep 20 2023, 6:47 PM

source/graphics/tests/test_Model.h
35–38 ↗	(On Diff #22259)	The shortest I came up with: const auto& map = defines.GetMap(); const auto it = map.find(define); return it != map.end() && it->second == define;