Visible/Non Visible Specifier Changes (#214)

* Visibility specifier changes to no longer underapproximate.

* Updated visible specifiers docs.

* Fixes and pitch tweaks.

* Fixes and test tweaks for new semantics.

* Fixed checkCyclical.

* Progress on pruning tweaks.

* Visiblity pruning progress.

* Work on Voxel to Mesh conversion.

* More visible spec changes.

* Voxel to Mesh Workaround

* Documentation edits.

* Update src/scenic/core/regions.py

Co-authored-by: Daniel Fremont <[email protected]>

* Test cleanup

* Test tweaks.

* Add WIP warnings to VoxelRegion.

* Added test for visibility pruning with offset.

* Ensure test to ensure offset is preserved after visibility pruning.

* Added validation tests.

* Added test for intersection region sampler.

* Test modification for extra coverage.

* Generic sampling tests/fixes.

* Various requested fixes.

* Apply suggestions from code review

Co-authored-by: Daniel Fremont <[email protected]>

* Docs updates.

* Update tests/syntax/test_specifiers.py

Co-authored-by: Daniel Fremont <[email protected]>

* Visibility test cleanup

* Voxel mesh cleanup and test.

* Voxel to mesh cleanup.

* Modified pitch_signs formatting.

---------

Co-authored-by: Daniel Fremont <[email protected]>

docs/new.rst

@@ -46,6 +46,8 @@ Backwards-incompatible semantics changes:
 
 	* The :specifier:`offset by` specifier now optionally specifies :prop:`parentOrientation`.
 
+	* The :specifier:`visible` and :specifier:`not visible` specifiers now take into account occlusion and the shapes of objects. In previous versions, they only checked whether the center of the object was visible/not visible, ignoring occlusion.
+
 Backwards-incompatible API changes:
 
 	* The **maxIterations** argument of `Simulator.simulate` now has default value 1, rather than 100.

docs/porting.rst

@@ -53,5 +53,7 @@ Specifically:
 
 * The specifier :specifier:`with heading {X}` is replaced with :specifier:`facing {X}`.
 
+* The :specifier:`visible` and :specifier:`not visible` will behave as they did in Scenic 2, requiring the *center* of the object to be visible rather than *any* part of the object. More precisely, :specifier:`visible` will specify :prop:`position` to be uniformly random in the observing object's :term:`visible region` and :specifier:`not visible` will specify :prop:`position` to be uniformly random in the difference of the :term:`workspace` and the observing object's visible region.
+
 Note that despite these changes, Scenic will still use 3D geometry internally.
 For example, if you write :scenic:`ego = new Object at (1, 2)` the value of :scenic:`ego.position` will be the 3D vector :scenic:`(1, 2, 0)`.

docs/reference/specifiers.rst

@@ -188,12 +188,19 @@ visible [from (*Point* | *OrientedPoint*)]
 	* :prop:`position` with priority 3
 	* also adds a requirement (see below)
 
-**Dependencies**: None
+**Dependencies**: :prop:`regionContainedIn`
 
 Requires that this object is visible from the :scenic:`ego` or the given `Point`/`OrientedPoint`. See the :ref:`Visibility System <visibility>` reference for a discussion of the visibility model.
 
-Also optionally specifies :prop:`position` to be a uniformly random point in the :term:`visible region` of the ego, or of the given Point/OrientedPoint if given.
-Note that the position set by this specifier is slightly stricter than simply adding a requirement that the ego :keyword:`can see` the object: the specifier makes the *center* of the object (its :prop:`position`) visible, while the :keyword:`can see` condition will be satisfied even if the center is not visible as long as some other part of the object is visible.
+Also optionally specifies :prop:`position` to be uniformly random over all points that could result in a visible object (note that the above requirement will ensure the object is in fact visible).
+
+.. versionchanged:: 3.0
+
+	This specifier now specifies :prop:`position` uniformly randomly over all points that could result in a visible object. This allows for objects whose :prop:`position` might be out of the visible region, but which have a portion of their occupied space visible (e.g. a corner that is visible). With the previous semantics, such configurations would never be generated because the *center* of the object was required to be visible.
+
+.. note::
+
+	As an implementation detail, :prop:`position` is initially set to be sampled from `everywhere` (or the :term:`workspace` if one has been set). Scenic will then attempt to further restrict the sample region via various pruning techniques, but sometimes this is not possible. If this occurs and Scenic has not been able to further restrict the sampled region from `everywhere`, an error will be raised at compile time. The simplest way to remedy this is by setting a workspace or specifying :prop:`position` with a higher priority using a different specifier.
 
 .. _not visible [from ({Point} | {OrientedPoint})]:
 
@@ -209,8 +216,11 @@ not visible [from (*Point* | *OrientedPoint*)]
 
 Requires that this object is *not* visible from the ego or the given `Point`/`OrientedPoint`.
 
-Similarly to :sampref:`visible [from ({Point} | {OrientedPoint})]`, this specifier can position the object uniformly at random in the *non-visible* region of the ego.
-However, it depends on :prop:`regionContainedIn`, in order to restrict the non-visible region to the :term:`container` of the object being created, which is hopefully a bounded region (if the non-visible region is unbounded, it cannot be uniformly sampled from and an error will be raised).
+Similarly to :sampref:`visible [from ({Point} | {OrientedPoint})]`, this specifier can optionally position the object uniformly at random over all points that could result in a non-visible object (note that the above requirement will ensure the object is in fact not visible).
+
+.. versionchanged:: 3.0
+
+	This specifier now specifies :prop:`position` uniformly randomly over all points that could result in a non-visible object. This disallows objects whose :prop:`position` is out of the visible region, but which have a portion of their occupied space visible (e.g. a corner that is visible). With the previous semantics, such configurations would sometimes be generated because only the *center* of the object was required to be non-visible.
 
 .. _(left | right) of {vector} [by {scalar}]:
 .. _left of:

docs/syntax_guide.rst

@@ -219,9 +219,9 @@ Additional specifiers for the :prop:`position` and :prop:`orientation` propertie
    * - :sampref:`beyond {vector} by ({vector} | {scalar}) [from ({vector} | {OrientedPoint})]`
      - Positions the object with respect to the line of sight from a point or the ego
    * - :sampref:`visible [from ({Point} | {OrientedPoint})]`
-     - Ensures the object is visible from the ego, or from the given Point/OrientedPoint if given, while optionally specifying position to be in the appropriate visible region.
+     - Ensures the object is visible from the ego, or from the given Point/OrientedPoint if given, while optionally specifying position to be uniformly random over all positions that result in a visible object.
    * - :sampref:`not visible [from ({Point} | {OrientedPoint})]`
-     - Ensures the object is not visible from the ego, or from the given Point/OrientedPoint if given, while optionally specifying position to be outside the appropriate visible region.
+     - Ensures the object is not visible from the ego, or from the given Point/OrientedPoint if given, while optionally specifying position to be uniformly random over all positions that result in a non-visible object.
    * - :sampref:`(left | right) of ({vector} | {OrientedPoint} | {Object}) [by {scalar}] <left of>`
      - Positions the object to the left/right by the given scalar distance.
    * - :sampref:`(ahead of | behind) ({vector} | {OrientedPoint} | {Object}) [by {scalar}] <ahead of>`

src/scenic/core/distributions.py

@@ -330,8 +330,8 @@ def __getattr__(self, name):
             return object.__getattribute__(self, name)
         return AttributeDistribution(name, self)
 
-    def __call__(self, *args):
-        return OperatorDistribution("__call__", self, args)
+    def __call__(self, *args, **kwargs):
+        return OperatorDistribution("__call__", self, args, kwargs)
 
     def __iter__(self):
         raise RandomControlFlowError(f"cannot iterate through a random value")
@@ -684,7 +684,7 @@ def supportInterval(self):
             return unionOfSupports(supportInterval(attr) for attr in attrs)
         return None, None
 
-    def __call__(self, *args):
+    def __call__(self, *args, **kwargs):
         vty = self.object._valueType
         retTy = None
         if vty is not object:
@@ -693,7 +693,7 @@ def __call__(self, *args):
                 if isinstance(func, property):
                     func = func.fget
                 retTy = get_type_hints(func).get("return")
-        return OperatorDistribution("__call__", self, args, valueType=retTy)
+        return OperatorDistribution("__call__", self, args, kwargs, valueType=retTy)
 
     def __repr__(self):
         return f"{self.object!r}.{self.attribute}"
@@ -704,15 +704,18 @@ class OperatorDistribution(Distribution):
 
     _deterministic = True
 
-    def __init__(self, operator, obj, operands, valueType=None):
+    def __init__(self, operator, obj, operands, kwoperands, valueType=None):
         operands = tuple(toDistribution(arg) for arg in operands)
+        kwoperands = {key: toDistribution(kwarg) for key, kwarg in kwoperands.items()}
+        dependencies = operands + tuple(kwoperands.values())
         if valueType is None:
             ty = type_support.underlyingType(obj)
-            valueType = self.inferType(ty, operator, operands)
-        super().__init__(obj, *operands, valueType=valueType)
+            valueType = self.inferType(ty, operator, operands, kwoperands)
+        super().__init__(obj, *dependencies, valueType=valueType)
         self.operator = operator
         self.object = obj
         self.operands = operands
+        self.kwoperands = kwoperands
         self.symbol = allowedReversibleOperators.get(operator)
         if self.symbol:
             if operator[:3] == "__r":
@@ -723,7 +726,7 @@ def __init__(self, operator, obj, operands, valueType=None):
             self.reverse = None
 
     @staticmethod
-    def inferType(ty, operator, operands):
+    def inferType(ty, operator, operands, kwoperands):
         """Attempt to infer the result type of the given operator application."""
         # If the object's type is known, see if we have a return type annotation.
         origin = type_support.get_type_origin(ty)
@@ -741,7 +744,9 @@ def inferType(ty, operator, operands):
                     # None does not support this operator; using it will raise an
                     # exception, so we can ignore this case for type inference.
                     continue
-                res = OperatorDistribution.inferType(option, operator, operands)
+                res = OperatorDistribution.inferType(
+                    option, operator, operands, kwoperands
+                )
                 types.append(res)
             return type_support.unifierOfTypes(types) if types else object
 
@@ -796,10 +801,11 @@ def scalar(thing):
     def sampleGiven(self, value):
         first = value[self.object]
         rest = [value[child] for child in self.operands]
+        kwargs = {key: value[child] for key, child in self.kwoperands.items()}
         op = getattr(first, self.operator)
-        result = op(*rest)
+        result = op(*rest, **kwargs)
         if result is NotImplemented and self.reverse:
-            assert len(rest) == 1
+            assert len(rest) == 1 and len(kwargs) == 0
             rop = getattr(rest[0], self.reverse)
             result = rop(first)
         if result is NotImplemented and self.symbol:
@@ -812,7 +818,10 @@ def sampleGiven(self, value):
     def evaluateInner(self, context):
         obj = valueInContext(self.object, context)
         operands = tuple(valueInContext(arg, context) for arg in self.operands)
-        return OperatorDistribution(self.operator, obj, operands)
+        kwoperands = {
+            key: valueInContext(arg, context) for key, kwarg in self.kwoperands.items()
+        }
+        return OperatorDistribution(self.operator, obj, operands, kwoperands)
 
     def supportInterval(self):
         if self.operator in (
@@ -825,7 +834,7 @@ def supportInterval(self):
             "__truediv__",
             "__rtruediv__",
         ):
-            assert len(self.operands) == 1
+            assert len(self.operands) == 1 and len(self.kwoperands) == 0
             l1, r1 = supportInterval(self.object)
             l2, r2 = supportInterval(self.operands[0])
             if l1 is None or l2 is None or r1 is None or r2 is None:
@@ -859,7 +868,7 @@ def supportInterval(self):
                 raise AssertionError(f"unexpected operator {self.operator}")
             return l, r
         elif self.operator in ("__neg__", "__abs__"):
-            assert len(self.operands) == 0
+            assert len(self.operands) == 0 and len(self.kwoperands) == 0
             l, r = supportInterval(self.object)
             if self.operator == "__neg__":
                 return -r, -l
@@ -921,7 +930,7 @@ def handler(self, arg):
                 and arg == 0
             ):
                 return self
-            return OperatorDistribution(op, self, (arg,), valueType=ty)
+            return OperatorDistribution(op, self, (arg,), {}, valueType=ty)
 
     elif op in ("__mul__", "__rmul__"):
 
@@ -933,7 +942,7 @@ def handler(self, arg):
 
                 if issubclass(self._valueType, Orientation) and arg == globalOrientation:
                     return self
-            return OperatorDistribution(op, self, (arg,), valueType=ty)
+            return OperatorDistribution(op, self, (arg,), {}, valueType=ty)
 
     elif op in ("__truediv__", "__floordiv__", "__pow__"):
 
@@ -944,12 +953,12 @@ def handler(self, arg):
                 and arg == 1
             ):
                 return self
-            return OperatorDistribution(op, self, (arg,), valueType=ty)
+            return OperatorDistribution(op, self, (arg,), {}, valueType=ty)
 
     else:
         # The general case.
         def handler(self, *args):
-            return OperatorDistribution(op, self, args, valueType=ty)
+            return OperatorDistribution(op, self, args, {}, valueType=ty)
 
     return handler
 

src/scenic/core/lazy_eval.py

@@ -262,7 +262,7 @@ def needsLazyEvaluation(thing):
 
 def dependencies(thing):
     """Dependencies which must be sampled before this value."""
-    return getattr(thing, "_dependencies", ())
+    return getattr(getattr(thing, "_conditioned", thing), "_dependencies", ())
 
 
 def needsSampling(thing):