Cycles network device fixes
ClosedPublic
Actions

Authored by Martijn Berger (juicyfruit) on Nov 23 2013, 8:34 PM.

Details

Reviewers

Commits

rBS85a0c5d4e103: Cycles: network render code updated for latest changes and improved
rCc902d18a485c: Cycles: network render code updated for latest changes and improved
rB85a0c5d4e103: Cycles: network render code updated for latest changes and improved

Summary

Code includes work by Doug Gregor (cleaned by me) and some little parts by myself.
We adhered to @Brecht Van Lommel (brecht) 's design as best we could.

This patch lacks scons support and is only tested with gcc and llvm on mac and linux

@Brecht Van Lommel (brecht) : I have removed the code touching the UI as we discussed on IRC last week

Diff Detail

Event Timeline

I should really be more careful instead of polluting phabricator. :(

Ah yes, we should get this into master. I will review this next week.

Did not review the code in device_network.cpp itself yet, but some initial comments.

intern/cycles/CMakeLists.txt
63	What is the purpose of this line? Doesn't seem to be needed.
intern/cycles/blender/addon/properties.py
31	This should either be commented out or there should be a a cycles.with_network property to check availability like there is a with_osl property.
intern/cycles/device/device.h
75–77	Cycles code style is to have these not indented and all on the same line. Same comment goes to the few other places that use this style.
intern/cycles/device/device_network.h
20	Why #if 1?
165–166	I don't think this is portable, would not compile with visual studio. This also seems like temporary debugging code or is it really intended to be here? If it's not debugging code, it's not clear to me why we would raise SIGTRAP, would prefer to handle errors a bit more gracefully and at least print an error message.
317	It's not clear to me why this line was changed, both would copy the vector the same way?
327	Not clear why error messages were removed.
402	This should be cycles_version, which may be a blender version for now but it's supposed to work standalone too.
intern/cycles/util/util_thread.h
65–68	This doesn't seem to be used.

Review the rest of the code now. I can't be totally sure this networking code is reliable, need to do a deeper analysis for that, but the additions seems reasonable and this is work in progress code, so if my comments are addressed this should be good to go in.

intern/cycles/device/device_network.cpp
281	This case is missing a lock.unlock();
640	This case i missing lock.unlock();
703	This case is missing a lock.unlock(); I know the lock going out of scope probably takes care of it, but that kind of thing can get missed when adding extra code to this function.