Alembic: simplified sub-frame sampling

It's now less confusing (for example, using nr_of_samples directly,
instead of using 1 / 1 / nr_of_samples). Might also have fixed a bug.

Also added unittests.

8 files changed, 168 insertions, 49 deletions

diff --git a/source/blender/alembic/ABC_alembic.h b/source/blender/alembic/ABC_alembic.h
index 6228ae60c56..70250310213 100644
--- a/source/blender/alembic/ABC_alembic.h
+++ b/source/blender/alembic/ABC_alembic.h
@@ -47,8 +47,8 @@ struct AlembicExportParams {
 	double frame_start;
 	double frame_end;
 
-	double frame_step_xform;
-	double frame_step_shape;
+	unsigned int frame_samples_xform;
+	unsigned int frame_samples_shape;
 
 	double shutter_open;
 	double shutter_close;
diff --git a/source/blender/alembic/intern/abc_exporter.cc b/source/blender/alembic/intern/abc_exporter.cc
index 0c0d73713fb..3da67ac0865 100644
--- a/source/blender/alembic/intern/abc_exporter.cc
+++ b/source/blender/alembic/intern/abc_exporter.cc
@@ -72,8 +72,8 @@ ExportSettings::ExportSettings()
     , renderable_only(false)
     , frame_start(1)
     , frame_end(1)
-    , frame_step_xform(1)
-    , frame_step_shape(1)
+    , frame_samples_xform(1)
+    , frame_samples_shape(1)
     , shutter_open(0.0)
     , shutter_close(1.0)
     , global_scale(1.0f)
@@ -189,31 +189,25 @@ AbcExporter::~AbcExporter()
 	delete m_writer;
 }
 
-void AbcExporter::getShutterSamples(double step, bool time_relative,
+void AbcExporter::getShutterSamples(unsigned int nr_of_samples,
+                                    bool time_relative,
                                     std::vector<double> &samples)
 {
+	Scene *scene = m_scene; /* for use in the FPS macro */
 	samples.clear();
 
-	const double time_factor = time_relative ? m_scene->r.frs_sec : 1.0;
-	const double shutter_open = m_settings.shutter_open;
-	const double shutter_close = m_settings.shutter_close;
+	unsigned int frame_offset = time_relative ? m_settings.frame_start : 0;
+	double time_factor = time_relative ? FPS : 1.0;
+	double shutter_open = m_settings.shutter_open;
+	double shutter_close = m_settings.shutter_close;
+	double time_inc = (shutter_close - shutter_open) / nr_of_samples;
 
-	/* sample all frame */
-	if (shutter_open == 0.0 && shutter_close == 1.0) {
-		for (double t = 0.0; t < 1.0; t += step) {
-			samples.push_back((t + m_settings.frame_start) / time_factor);
-		}
-	}
-	else {
-		/* sample between shutter open & close */
-		const int nsamples = static_cast<int>(std::max((1.0 / step) - 1.0, 1.0));
-		const double time_inc = (shutter_close - shutter_open) / nsamples;
-
-		for (int sample=0; sample < nsamples; ++sample) {
-			double sample_time = shutter_open + time_inc * sample;
-			double time = (m_settings.frame_start + sample_time) / time_factor;
-			samples.push_back(time);
-		}
+	/* sample between shutter open & close */
+	for (int sample=0; sample < nr_of_samples; ++sample) {
+		double sample_time = shutter_open + time_inc * sample;
+		double time = (frame_offset + sample_time) / time_factor;
+
+		samples.push_back(time);
 	}
 }
 
@@ -227,21 +221,24 @@ Alembic::Abc::TimeSamplingPtr AbcExporter::createTimeSampling(double step)
 
 	getShutterSamples(step, true, samples);
 
-	Alembic::Abc::TimeSamplingType ts(static_cast<uint32_t>(samples.size()), 1.0 / m_scene->r.frs_sec);
+	Alembic::Abc::TimeSamplingType ts(
+	            static_cast<uint32_t>(samples.size()),
+	            1.0 / m_scene->r.frs_sec);
 
 	return TimeSamplingPtr(new Alembic::Abc::TimeSampling(ts, samples));
 }
 
-void AbcExporter::getFrameSet(double step, std::set<double> &frames)
+void AbcExporter::getFrameSet(unsigned int nr_of_samples,
+                              std::set<double> &frames)
 {
 	frames.clear();
 
 	std::vector<double> shutter_samples;
 
-	getShutterSamples(step, false, shutter_samples);
+	getShutterSamples(nr_of_samples, false, shutter_samples);
 
 	for (double frame = m_settings.frame_start; frame <= m_settings.frame_end; frame += 1.0) {
-		for (int j = 0, e = shutter_samples.size(); j < e; ++j) {
+		for (size_t j = 0; j < nr_of_samples; ++j) {
 			frames.insert(frame + shutter_samples[j]);
 		}
 	}
@@ -273,20 +270,20 @@ void AbcExporter::operator()(Main *bmain, float &progress, bool &was_canceled)
 
 	/* Create time samplings for transforms and shapes. */
 
-	TimeSamplingPtr trans_time = createTimeSampling(m_settings.frame_step_xform);
+	TimeSamplingPtr trans_time = createTimeSampling(m_settings.frame_samples_xform);
 
 	m_trans_sampling_index = m_writer->archive().addTimeSampling(*trans_time);
 
 	TimeSamplingPtr shape_time;
 
-	if ((m_settings.frame_step_shape == m_settings.frame_step_xform) ||
+	if ((m_settings.frame_samples_shape == m_settings.frame_samples_xform) ||
 	    (m_settings.frame_start == m_settings.frame_end))
 	{
 		shape_time = trans_time;
 		m_shape_sampling_index = m_trans_sampling_index;
 	}
 	else {
-		shape_time = createTimeSampling(m_settings.frame_step_shape);
+		shape_time = createTimeSampling(m_settings.frame_samples_shape);
 		m_shape_sampling_index = m_writer->archive().addTimeSampling(*shape_time);
 	}
 
@@ -298,13 +295,12 @@ void AbcExporter::operator()(Main *bmain, float &progress, bool &was_canceled)
 	/* Make a list of frames to export. */
 
 	std::set<double> xform_frames;
-	getFrameSet(m_settings.frame_step_xform, xform_frames);
+	getFrameSet(m_settings.frame_samples_xform, xform_frames);
 
 	std::set<double> shape_frames;
-	getFrameSet(m_settings.frame_step_shape, shape_frames);
+	getFrameSet(m_settings.frame_samples_shape, shape_frames);
 
 	/* Merge all frames needed. */
-
 	std::set<double> frames(xform_frames);
 	frames.insert(shape_frames.begin(), shape_frames.end());
 
@@ -327,7 +323,7 @@ void AbcExporter::operator()(Main *bmain, float &progress, bool &was_canceled)
 		const double frame = *begin;
 
 		/* 'frame' is offset by start frame, so need to cancel the offset. */
-		setCurrentFrame(bmain, frame - m_settings.frame_start);
+		setCurrentFrame(bmain, frame);
 
 		if (shape_frames.count(frame) != 0) {
 			for (int i = 0, e = m_shapes.size(); i != e; ++i) {
diff --git a/source/blender/alembic/intern/abc_exporter.h b/source/blender/alembic/intern/abc_exporter.h
index 797a2560d65..f763922a73b 100644
--- a/source/blender/alembic/intern/abc_exporter.h
+++ b/source/blender/alembic/intern/abc_exporter.h
@@ -50,8 +50,8 @@ struct ExportSettings {
 	bool renderable_only;
 
 	double frame_start, frame_end;
-	double frame_step_xform;
-	double frame_step_shape;
+	double frame_samples_xform;
+	double frame_samples_shape;
 	double shutter_open;
 	double shutter_close;
 	float global_scale;
@@ -103,13 +103,15 @@ public:
 
 	void operator()(Main *bmain, float &progress, bool &was_canceled);
 
-private:
-	void getShutterSamples(double step, bool time_relative, std::vector<double> &samples);
+protected:
+	void getShutterSamples(unsigned int nr_of_samples,
+	                       bool time_relative,
+	                       std::vector<double> &samples);
+	void getFrameSet(unsigned int nr_of_samples, std::set<double> &frames);
 
+private:
 	Alembic::Abc::TimeSamplingPtr createTimeSampling(double step);
 
-	void getFrameSet(double step, std::set<double> &frames);
-
 	void createTransformWritersHierarchy(EvaluationContext *eval_ctx);
 	AbcTransformWriter * createTransformWriter(Object *ob,  Object *parent, Object *dupliObParent);
 	void exploreTransform(EvaluationContext *eval_ctx, Object *ob, Object *parent, Object *dupliObParent = NULL);
diff --git a/source/blender/alembic/intern/alembic_capi.cc b/source/blender/alembic/intern/alembic_capi.cc
index 52864799133..cb45ee643cd 100644
--- a/source/blender/alembic/intern/alembic_capi.cc
+++ b/source/blender/alembic/intern/alembic_capi.cc
@@ -332,8 +332,8 @@ bool ABC_export(
 	job->settings.scene = job->scene;
 	job->settings.frame_start = params->frame_start;
 	job->settings.frame_end = params->frame_end;
-	job->settings.frame_step_xform = params->frame_step_xform;
-	job->settings.frame_step_shape = params->frame_step_shape;
+	job->settings.frame_samples_xform = params->frame_samples_xform;
+	job->settings.frame_samples_shape = params->frame_samples_shape;
 	job->settings.shutter_open = params->shutter_open;
 	job->settings.shutter_close = params->shutter_close;
 	job->settings.selected_only = params->selected_only;
diff --git a/source/blender/editors/io/io_alembic.c b/source/blender/editors/io/io_alembic.c
index 3da4caa7cfc..ba8792d12ff 100644
--- a/source/blender/editors/io/io_alembic.c
+++ b/source/blender/editors/io/io_alembic.c
@@ -106,8 +106,8 @@ static int wm_alembic_export_exec(bContext *C, wmOperator *op)
 	    .frame_start = RNA_int_get(op->ptr, "start"),
 	    .frame_end = RNA_int_get(op->ptr, "end"),
 
-	    .frame_step_xform = 1.0 / (double)RNA_int_get(op->ptr, "xsamples"),
-	    .frame_step_shape = 1.0 / (double)RNA_int_get(op->ptr, "gsamples"),
+	    .frame_samples_xform = RNA_int_get(op->ptr, "xsamples"),
+	    .frame_samples_shape = RNA_int_get(op->ptr, "gsamples"),
 
 	    .shutter_open = RNA_float_get(op->ptr, "sh_open"),
 	    .shutter_close = RNA_float_get(op->ptr, "sh_close"),
diff --git a/source/blender/makesrna/intern/rna_scene_api.c b/source/blender/makesrna/intern/rna_scene_api.c
index d3487fca42e..a80e4c60127 100644
--- a/source/blender/makesrna/intern/rna_scene_api.c
+++ b/source/blender/makesrna/intern/rna_scene_api.c
@@ -227,8 +227,8 @@ static void rna_Scene_alembic_export(
 	    .frame_start = frame_start,
 	    .frame_end = frame_end,
 
-	    .frame_step_xform = 1.0 / (double)xform_samples,
-	    .frame_step_shape = 1.0 / (double)geom_samples,
+	    .frame_samples_xform = xform_samples,
+	    .frame_samples_shape = geom_samples,
 
 	    .shutter_open = shutter_open,
 	    .shutter_close = shutter_close,
diff --git a/tests/gtests/alembic/CMakeLists.txt b/tests/gtests/alembic/CMakeLists.txt
index c1480910d42..fadf549e212 100644
--- a/tests/gtests/alembic/CMakeLists.txt
+++ b/tests/gtests/alembic/CMakeLists.txt
@@ -26,6 +26,7 @@ set(INC
 	..
 	../../../source/blender/blenlib
 	../../../source/blender/alembic
+	../../../source/blender/makesdna
 	${ALEMBIC_INCLUDE_DIRS}
 	${BOOST_INCLUDE_DIR}
 	${HDF5_INCLUDE_DIRS}
@@ -44,8 +45,8 @@ else()
 endif()
 
 # For motivation on doubling BLENDER_SORTED_LIBS, see ../bmesh/CMakeLists.txt
-BLENDER_SRC_GTEST(abc_matrix "abc_matrix_test.cc;${_buildinfo_src}" "${BLENDER_SORTED_LIBS};${BLENDER_SORTED_LIBS}")
+BLENDER_SRC_GTEST(alembic "abc_matrix_test.cc;abc_export_test.cc;${_buildinfo_src}" "${BLENDER_SORTED_LIBS};${BLENDER_SORTED_LIBS}")
 
 unset(_buildinfo_src)
 
-setup_liblinks(abc_matrix_test)
+setup_liblinks(alembic_test)
diff --git a/tests/gtests/alembic/abc_export_test.cc b/tests/gtests/alembic/abc_export_test.cc
new file mode 100644
index 00000000000..63c1d179e51
--- /dev/null
+++ b/tests/gtests/alembic/abc_export_test.cc
@@ -0,0 +1,120 @@
+#include "testing/testing.h"
+
+// Keep first since utildefines defines AT which conflicts with fucking STL
+#include "intern/abc_util.h"
+#include "intern/abc_exporter.h"
+
+extern "C" {
+#include "BLI_utildefines.h"
+#include "BLI_math.h"
+#include "DNA_scene_types.h"
+}
+
+class TestableAbcExporter : public AbcExporter {
+public:
+	TestableAbcExporter(Scene *scene, const char *filename, ExportSettings &settings)
+	    : AbcExporter(scene, filename, settings)
+	{}
+
+	void getShutterSamples(unsigned int nr_of_samples,
+	                       bool time_relative,
+	                       std::vector<double> &samples)
+	{
+		AbcExporter::getShutterSamples(nr_of_samples, time_relative, samples);
+	}
+
+	void getFrameSet(unsigned int nr_of_samples,
+	                 std::set<double> &frames) {
+		AbcExporter::getFrameSet(nr_of_samples, frames);
+	}
+
+};
+
+
+TEST(abc_export, TimeSamplesFullShutter) {
+	ExportSettings settings;
+	settings.frame_start = 31.0;
+	settings.frame_end = 223.0;
+	settings.shutter_open = 0.0;
+	settings.shutter_close = 1.0;
+
+	/* Fake a 25 FPS scene with a nonzero base (because that's sometimes forgotten) */
+	Scene scene;
+	scene.r.frs_sec = 50;
+	scene.r.frs_sec_base = 2;
+
+	TestableAbcExporter exporter(&scene, "somefile.abc", settings);
+	std::vector<double> samples;
+
+	/* test 5 samples per frame */
+	exporter.getShutterSamples(5, true, samples);
+	EXPECT_EQ(5, samples.size());
+	EXPECT_NEAR(1.240, samples[0], 1e-5f);
+	EXPECT_NEAR(1.248, samples[1], 1e-5f);
+	EXPECT_NEAR(1.256, samples[2], 1e-5f);
+	EXPECT_NEAR(1.264, samples[3], 1e-5f);
+	EXPECT_NEAR(1.272, samples[4], 1e-5f);
+
+	/* test same, but using frame number offset instead of time */
+	exporter.getShutterSamples(5, false, samples);
+	EXPECT_EQ(5, samples.size());
+	EXPECT_NEAR(0.0, samples[0], 1e-5f);
+	EXPECT_NEAR(0.2, samples[1], 1e-5f);
+	EXPECT_NEAR(0.4, samples[2], 1e-5f);
+	EXPECT_NEAR(0.6, samples[3], 1e-5f);
+	EXPECT_NEAR(0.8, samples[4], 1e-5f);
+
+	/* use the same setup to test getFrameSet() */
+	std::set<double> frames;
+	exporter.getFrameSet(5, frames);
+	EXPECT_EQ(965, frames.size());
+	EXPECT_EQ(1, frames.count(31.0));
+	EXPECT_EQ(1, frames.count(31.2));
+	EXPECT_EQ(1, frames.count(31.4));
+	EXPECT_EQ(1, frames.count(31.6));
+	EXPECT_EQ(1, frames.count(31.8));
+}
+
+
+TEST(abc_export, TimeSamples180degShutter) {
+	ExportSettings settings;
+	settings.frame_start = 31.0;
+	settings.frame_end = 223.0;
+	settings.shutter_open = -0.25;
+	settings.shutter_close = 0.25;
+
+	/* Fake a 25 FPS scene with a nonzero base (because that's sometimes forgotten) */
+	Scene scene;
+	scene.r.frs_sec = 50;
+	scene.r.frs_sec_base = 2;
+
+	TestableAbcExporter exporter(&scene, "somefile.abc", settings);
+	std::vector<double> samples;
+
+	/* test 5 samples per frame */
+	exporter.getShutterSamples(5, true, samples);
+	EXPECT_EQ(5, samples.size());
+	EXPECT_NEAR(1.230, samples[0], 1e-5f);
+	EXPECT_NEAR(1.234, samples[1], 1e-5f);
+	EXPECT_NEAR(1.238, samples[2], 1e-5f);
+	EXPECT_NEAR(1.242, samples[3], 1e-5f);
+	EXPECT_NEAR(1.246, samples[4], 1e-5f);
+
+	/* test same, but using frame number offset instead of time */
+	exporter.getShutterSamples(5, false, samples);
+	EXPECT_EQ(5, samples.size());
+	EXPECT_NEAR(-0.25, samples[0], 1e-5f);
+	EXPECT_NEAR(-0.15, samples[1], 1e-5f);
+	EXPECT_NEAR(-0.05, samples[2], 1e-5f);
+	EXPECT_NEAR( 0.05, samples[3], 1e-5f);
+	EXPECT_NEAR( 0.15, samples[4], 1e-5f);
+
+	/* Use the same setup to test getFrameSet().
+	 * Here only a few numbers are tested, due to rounding issues. */
+	std::set<double> frames;
+	exporter.getFrameSet(5, frames);
+	EXPECT_EQ(965, frames.size());
+	EXPECT_EQ(1, frames.count(30.75));
+	EXPECT_EQ(1, frames.count(30.95));
+	EXPECT_EQ(1, frames.count(31.15));
+}