Merge pull request #9526 from highfidelity/edit-entity-filter

Edit entity filter (from branch)
This commit is contained in:
Howard Stearns 2017-01-26 16:00:48 -08:00 committed by GitHub
commit 866f9debcd
11 changed files with 270 additions and 21 deletions

View file

@ -9,9 +9,12 @@
// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
//
#include <QtCore/QEventLoop>
#include <QTimer>
#include <EntityTree.h>
#include <SimpleEntitySimulation.h>
#include <ResourceCache.h>
#include <ScriptCache.h>
#include "EntityServer.h"
#include "EntityServerConsts.h"
@ -26,6 +29,10 @@ EntityServer::EntityServer(ReceivedMessage& message) :
OctreeServer(message),
_entitySimulation(NULL)
{
ResourceManager::init();
DependencyManager::set<ResourceCacheSharedItems>();
DependencyManager::set<ScriptCache>();
auto& packetReceiver = DependencyManager::get<NodeList>()->getPacketReceiver();
packetReceiver.registerListenerForTypes({ PacketType::EntityAdd, PacketType::EntityEdit, PacketType::EntityErase },
this, "handleEntityPacket");
@ -285,6 +292,97 @@ void EntityServer::readAdditionalConfiguration(const QJsonObject& settingsSectio
} else {
tree->setEntityScriptSourceWhitelist("");
}
if (readOptionString("entityEditFilter", settingsSectionObject, _entityEditFilter) && !_entityEditFilter.isEmpty()) {
// Fetch script from file synchronously. We don't want the server processing edits while a restarting entity server is fetching from a DOS'd source.
QUrl scriptURL(_entityEditFilter);
// The following should be abstracted out for use in Agent.cpp (and maybe later AvatarMixer.cpp)
if (scriptURL.scheme().isEmpty() || (scriptURL.scheme() == URL_SCHEME_FILE)) {
qWarning() << "Cannot load script from local filesystem, because assignment may be on a different computer.";
scriptRequestFinished();
return;
}
auto scriptRequest = ResourceManager::createResourceRequest(this, scriptURL);
if (!scriptRequest) {
qWarning() << "Could not create ResourceRequest for Agent script at" << scriptURL.toString();
scriptRequestFinished();
return;
}
// Agent.cpp sets up a timeout here, but that is unnecessary, as ResourceRequest has its own.
connect(scriptRequest, &ResourceRequest::finished, this, &EntityServer::scriptRequestFinished);
// FIXME: handle atp rquests setup here. See Agent::requestScript()
qInfo() << "Requesting script at URL" << qPrintable(scriptRequest->getUrl().toString());
scriptRequest->send();
_scriptRequestLoop.exec(); // Block here, but allow the request to be processed and its signals to be handled.
}
}
// Copied from ScriptEngine.cpp. We should make this a class method for reuse.
// Note: I've deliberately stopped short of using ScriptEngine instead of QScriptEngine, as that is out of project scope at this point.
static bool hasCorrectSyntax(const QScriptProgram& program) {
const auto syntaxCheck = QScriptEngine::checkSyntax(program.sourceCode());
if (syntaxCheck.state() != QScriptSyntaxCheckResult::Valid) {
const auto error = syntaxCheck.errorMessage();
const auto line = QString::number(syntaxCheck.errorLineNumber());
const auto column = QString::number(syntaxCheck.errorColumnNumber());
const auto message = QString("[SyntaxError] %1 in %2:%3(%4)").arg(error, program.fileName(), line, column);
qCritical() << qPrintable(message);
return false;
}
return true;
}
static bool hadUncaughtExceptions(QScriptEngine& engine, const QString& fileName) {
if (engine.hasUncaughtException()) {
const auto backtrace = engine.uncaughtExceptionBacktrace();
const auto exception = engine.uncaughtException().toString();
const auto line = QString::number(engine.uncaughtExceptionLineNumber());
engine.clearExceptions();
static const QString SCRIPT_EXCEPTION_FORMAT = "[UncaughtException] %1 in %2:%3";
auto message = QString(SCRIPT_EXCEPTION_FORMAT).arg(exception, fileName, line);
if (!backtrace.empty()) {
static const auto lineSeparator = "\n ";
message += QString("\n[Backtrace]%1%2").arg(lineSeparator, backtrace.join(lineSeparator));
}
qCritical() << qPrintable(message);
return true;
}
return false;
}
void EntityServer::scriptRequestFinished() {
auto scriptRequest = qobject_cast<ResourceRequest*>(sender());
const QString urlString = scriptRequest->getUrl().toString();
if (scriptRequest && scriptRequest->getResult() == ResourceRequest::Success) {
auto scriptContents = scriptRequest->getData();
qInfo() << "Downloaded script:" << scriptContents;
QScriptProgram program(scriptContents, urlString);
if (hasCorrectSyntax(program)) {
_entityEditFilterEngine.evaluate(scriptContents);
if (!hadUncaughtExceptions(_entityEditFilterEngine, urlString)) {
std::static_pointer_cast<EntityTree>(_tree)->initEntityEditFilterEngine(&_entityEditFilterEngine, [this]() {
return hadUncaughtExceptions(_entityEditFilterEngine, _entityEditFilter);
});
scriptRequest->deleteLater();
if (_scriptRequestLoop.isRunning()) {
_scriptRequestLoop.quit();
}
return;
}
}
} else if (scriptRequest) {
qCritical() << "Failed to download script at" << urlString;
// See HTTPResourceRequest::onRequestFinished for interpretation of codes. For example, a 404 is code 6 and 403 is 3. A timeout is 2. Go figure.
qCritical() << "ResourceRequest error was" << scriptRequest->getResult();
} else {
qCritical() << "Failed to create script request.";
}
// Hard stop of the assignment client on failure. We don't want anyone to think they have a filter in place when they don't.
// Alas, only indications will be the above logging with assignment client restarting repeatedly, and clients will not see any entities.
stop();
if (_scriptRequestLoop.isRunning()) {
_scriptRequestLoop.quit();
}
}
void EntityServer::nodeAdded(SharedNodePointer node) {

View file

@ -69,6 +69,7 @@ protected:
private slots:
void handleEntityPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer senderNode);
void scriptRequestFinished();
private:
SimpleEntitySimulationPointer _entitySimulation;
@ -76,6 +77,10 @@ private:
QReadWriteLock _viewerSendingStatsLock;
QMap<QUuid, QMap<QUuid, ViewerSendingStats>> _viewerSendingStats;
QString _entityEditFilter{};
QScriptEngine _entityEditFilterEngine{};
QEventLoop _scriptRequestLoop{};
};
#endif // hifi_EntityServer_h

View file

@ -660,6 +660,7 @@ bool OctreeServer::handleHTTPRequest(HTTPConnection* connection, const QUrl& url
quint64 averageUpdateTime = _tree->getAverageUpdateTime();
quint64 averageCreateTime = _tree->getAverageCreateTime();
quint64 averageLoggingTime = _tree->getAverageLoggingTime();
quint64 averageFilterTime = _tree->getAverageFilterTime();
int FLOAT_PRECISION = 3;
@ -699,6 +700,8 @@ bool OctreeServer::handleHTTPRequest(HTTPConnection* connection, const QUrl& url
.arg(locale.toString((uint)averageCreateTime).rightJustified(COLUMN_WIDTH, ' '));
statsString += QString(" Average Logging Time: %1 usecs\r\n")
.arg(locale.toString((uint)averageLoggingTime).rightJustified(COLUMN_WIDTH, ' '));
statsString += QString(" Average Filter Time: %1 usecs\r\n")
.arg(locale.toString((uint)averageFilterTime).rightJustified(COLUMN_WIDTH, ' '));
int senderNumber = 0;

View file

@ -1290,6 +1290,14 @@
"default": "",
"advanced": true
},
{
"name": "entityEditFilter",
"label": "Filter Entity Edits",
"help": "Check all entity edits against this filter function.",
"placeholder": "url whose content is like: function filter(properties) { return properties; }",
"default": "",
"advanced": true
},
{
"name": "persistFilePath",
"label": "Entities File Path",

View file

@ -351,7 +351,6 @@ int EntityItem::expectedBytes() {
return MINIMUM_HEADER_BYTES;
}
// clients use this method to unpack FULL updates from entity-server
int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLeftToRead, ReadBitstreamToTreeParams& args) {
if (args.bitstreamVersion < VERSION_ENTITIES_SUPPORT_SPLIT_MTU) {
@ -669,6 +668,9 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef
// entity-server is trying to clear our ownership (probably at our own request)
// but we actually want to own it, therefore we ignore this clear event
// and pretend that we own it (we assume we'll recover it soon)
// However, for now, when the server uses a newer time than what we sent, listen to what we're told.
if (overwriteLocalData) weOwnSimulation = false;
} else if (_simulationOwner.set(newSimOwner)) {
_dirtyFlags |= Simulation::DIRTY_SIMULATOR_ID;
somethingChanged = true;

View file

@ -347,11 +347,15 @@ EntityPropertyFlags EntityItemProperties::getChangedProperties() const {
return changedProperties;
}
QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool skipDefaults) const {
QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool skipDefaults, bool allowUnknownCreateTime, bool strictSemantics) const {
// If strictSemantics is true and skipDefaults is false, then all and only those properties are copied for which the property flag
// is included in _desiredProperties, or is one of the specially enumerated ALWAYS properties below.
// (There may be exceptions, but if so, they are bugs.)
// In all other cases, you are welcome to inspect the code and try to figure out what was intended. I wish you luck. -HRS 1/18/17
QScriptValue properties = engine->newObject();
EntityItemProperties defaultEntityProperties;
if (_created == UNKNOWN_CREATED_TIME) {
if (_created == UNKNOWN_CREATED_TIME && !allowUnknownCreateTime) {
// No entity properties can have been set so return without setting any default, zero property values.
return properties;
}
@ -365,7 +369,7 @@ QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool
created.setTimeSpec(Qt::OffsetFromUTC);
COPY_PROPERTY_TO_QSCRIPTVALUE_GETTER_ALWAYS(created, created.toString(Qt::ISODate));
if (!skipDefaults || _lifetime != defaultEntityProperties._lifetime) {
if ((!skipDefaults || _lifetime != defaultEntityProperties._lifetime) && !strictSemantics) {
COPY_PROPERTY_TO_QSCRIPTVALUE_GETTER_NO_SKIP(age, getAge()); // gettable, but not settable
COPY_PROPERTY_TO_QSCRIPTVALUE_GETTER_NO_SKIP(ageAsText, formatSecondsElapsed(getAge())); // gettable, but not settable
}
@ -541,7 +545,7 @@ QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool
}
// Sitting properties support
if (!skipDefaults) {
if (!skipDefaults && !strictSemantics) {
QScriptValue sittingPoints = engine->newObject();
for (int i = 0; i < _sittingPoints.size(); ++i) {
QScriptValue sittingPoint = engine->newObject();
@ -554,7 +558,7 @@ QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool
COPY_PROPERTY_TO_QSCRIPTVALUE_GETTER_ALWAYS(sittingPoints, sittingPoints); // gettable, but not settable
}
if (!skipDefaults) {
if (!skipDefaults && !strictSemantics) {
AABox aaBox = getAABox();
QScriptValue boundingBox = engine->newObject();
QScriptValue bottomRightNear = vec3toScriptValue(engine, aaBox.getCorner());
@ -569,7 +573,7 @@ QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool
}
QString textureNamesStr = QJsonDocument::fromVariant(_textureNames).toJson();
if (!skipDefaults) {
if (!skipDefaults && !strictSemantics) {
COPY_PROPERTY_TO_QSCRIPTVALUE_GETTER_NO_SKIP(originalTextures, textureNamesStr); // gettable, but not settable
}
@ -586,7 +590,7 @@ QScriptValue EntityItemProperties::copyToScriptValue(QScriptEngine* engine, bool
COPY_PROPERTY_TO_QSCRIPTVALUE(PROP_OWNING_AVATAR_ID, owningAvatarID);
// Rendering info
if (!skipDefaults) {
if (!skipDefaults && !strictSemantics) {
QScriptValue renderInfo = engine->newObject();
// currently only supported by models

View file

@ -73,7 +73,7 @@ public:
EntityTypes::EntityType getType() const { return _type; }
void setType(EntityTypes::EntityType type) { _type = type; }
virtual QScriptValue copyToScriptValue(QScriptEngine* engine, bool skipDefaults) const;
virtual QScriptValue copyToScriptValue(QScriptEngine* engine, bool skipDefaults, bool allowUnknownCreateTime = false, bool strictSemantics = false) const;
virtual void copyFromScriptValue(const QScriptValue& object, bool honorReadOnly);
static QScriptValue entityPropertyFlagsToScriptValue(QScriptEngine* engine, const EntityPropertyFlags& flags);
@ -93,6 +93,8 @@ public:
void debugDump() const;
void setLastEdited(quint64 usecTime);
EntityPropertyFlags getDesiredProperties() { return _desiredProperties; }
void setDesiredProperties(EntityPropertyFlags properties) { _desiredProperties = properties; }
// Note: DEFINE_PROPERTY(PROP_FOO, Foo, foo, type, value) creates the following methods and variables:
// type getFoo() const;
@ -462,10 +464,6 @@ inline QDebug operator<<(QDebug debug, const EntityItemProperties& properties) {
DEBUG_PROPERTY_IF_CHANGED(debug, properties, LastEditedBy, lastEditedBy, "");
properties.getAnimation().debugDump();
properties.getSkybox().debugDump();
properties.getStage().debugDump();
debug << " last edited:" << properties.getLastEdited() << "\n";
debug << " edited ago:" << properties.getEditedAgo() << "\n";
debug << "]";

View file

@ -922,6 +922,56 @@ void EntityTree::fixupTerseEditLogging(EntityItemProperties& properties, QList<Q
}
}
void EntityTree::initEntityEditFilterEngine(QScriptEngine* engine, std::function<bool()> entityEditFilterHadUncaughtExceptions) {
_entityEditFilterEngine = engine;
_entityEditFilterHadUncaughtExceptions = entityEditFilterHadUncaughtExceptions;
auto global = _entityEditFilterEngine->globalObject();
_entityEditFilterFunction = global.property("filter");
_hasEntityEditFilter = _entityEditFilterFunction.isFunction();
}
bool EntityTree::filterProperties(EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged) {
if (!_hasEntityEditFilter || !_entityEditFilterEngine) {
propertiesOut = propertiesIn;
wasChanged = false; // not changed
return true; // allowed
}
auto oldProperties = propertiesIn.getDesiredProperties();
auto specifiedProperties = propertiesIn.getChangedProperties();
propertiesIn.setDesiredProperties(specifiedProperties);
QScriptValue inputValues = propertiesIn.copyToScriptValue(_entityEditFilterEngine, false, true, true);
propertiesIn.setDesiredProperties(oldProperties);
auto in = QJsonValue::fromVariant(inputValues.toVariant()); // grab json copy now, because the inputValues might be side effected by the filter.
QScriptValueList args;
args << inputValues;
QScriptValue result = _entityEditFilterFunction.call(_nullObjectForFilter, args);
if (_entityEditFilterHadUncaughtExceptions()) {
result = QScriptValue();
}
bool accepted = result.isObject(); // filters should return null or false to completely reject edit or add
if (accepted) {
propertiesOut.copyFromScriptValue(result, false);
// Javascript objects are == only if they are the same object. To compare arbitrary values, we need to use JSON.
auto out = QJsonValue::fromVariant(result.toVariant());
wasChanged = in != out;
}
return accepted;
}
void EntityTree::bumpTimestamp(EntityItemProperties& properties) { //fixme put class/header
const quint64 LAST_EDITED_SERVERSIDE_BUMP = 1; // usec
// also bump up the lastEdited time of the properties so that the interface that created this edit
// will accept our adjustment to lifetime back into its own entity-tree.
if (properties.getLastEdited() == UNKNOWN_CREATED_TIME) {
properties.setLastEdited(usecTimestampNow());
}
properties.setLastEdited(properties.getLastEdited() + LAST_EDITED_SERVERSIDE_BUMP);
}
int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned char* editData, int maxLength,
const SharedNodePointer& senderNode) {
@ -945,9 +995,9 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
quint64 startLookup = 0, endLookup = 0;
quint64 startUpdate = 0, endUpdate = 0;
quint64 startCreate = 0, endCreate = 0;
quint64 startFilter = 0, endFilter = 0;
quint64 startLogging = 0, endLogging = 0;
const quint64 LAST_EDITED_SERVERSIDE_BUMP = 1; // usec
bool suppressDisallowedScript = false;
_totalEditMessages++;
@ -999,18 +1049,28 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
if (properties.getLifetime() == ENTITY_ITEM_IMMORTAL_LIFETIME ||
properties.getLifetime() > _maxTmpEntityLifetime) {
properties.setLifetime(_maxTmpEntityLifetime);
// also bump up the lastEdited time of the properties so that the interface that created this edit
// will accept our adjustment to lifetime back into its own entity-tree.
if (properties.getLastEdited() == UNKNOWN_CREATED_TIME) {
properties.setLastEdited(usecTimestampNow());
}
properties.setLastEdited(properties.getLastEdited() + LAST_EDITED_SERVERSIDE_BUMP);
bumpTimestamp(properties);
}
}
// If we got a valid edit packet, then it could be a new entity or it could be an update to
// an existing entity... handle appropriately
if (validEditPacket) {
startFilter = usecTimestampNow();
bool wasChanged = false;
// Having (un)lock rights bypasses the filter.
bool allowed = senderNode->isAllowedEditor() || filterProperties(properties, properties, wasChanged);
if (!allowed) {
properties = EntityItemProperties();
}
if (!allowed || wasChanged) {
bumpTimestamp(properties);
// For now, free ownership on any modification.
properties.clearSimulationOwner();
}
endFilter = usecTimestampNow();
// search for the entity by EntityItemID
startLookup = usecTimestampNow();
EntityItemPointer existingEntity = findEntityByEntityItemID(entityItemID);
@ -1018,7 +1078,7 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
if (existingEntity && message.getType() == PacketType::EntityEdit) {
if (suppressDisallowedScript) {
properties.setLastEdited(properties.getLastEdited() + LAST_EDITED_SERVERSIDE_BUMP);
bumpTimestamp(properties);
properties.setScript(existingEntity->getScript());
}
@ -1088,6 +1148,7 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c
_totalUpdateTime += endUpdate - startUpdate;
_totalCreateTime += endCreate - startCreate;
_totalLoggingTime += endLogging - startLogging;
_totalFilterTime += endFilter - startFilter;
break;
}

View file

@ -239,6 +239,7 @@ public:
virtual quint64 getAverageUpdateTime() const override { return _totalUpdates == 0 ? 0 : _totalUpdateTime / _totalUpdates; }
virtual quint64 getAverageCreateTime() const override { return _totalCreates == 0 ? 0 : _totalCreateTime / _totalCreates; }
virtual quint64 getAverageLoggingTime() const override { return _totalEditMessages == 0 ? 0 : _totalLoggingTime / _totalEditMessages; }
virtual quint64 getAverageFilterTime() const override { return _totalEditMessages == 0 ? 0 : _totalFilterTime / _totalEditMessages; }
void trackIncomingEntityLastEdited(quint64 lastEditedTime, int bytesRead);
quint64 getAverageEditDeltas() const
@ -265,6 +266,8 @@ public:
void notifyNewCollisionSoundURL(const QString& newCollisionSoundURL, const EntityItemID& entityID);
void initEntityEditFilterEngine(QScriptEngine* engine, std::function<bool()> entityEditFilterHadUncaughtExceptions);
static const float DEFAULT_MAX_TMP_ENTITY_LIFETIME;
public slots:
@ -290,6 +293,7 @@ protected:
static bool findInBoxOperation(OctreeElementPointer element, void* extraData);
static bool findInFrustumOperation(OctreeElementPointer element, void* extraData);
static bool sendEntitiesOperation(OctreeElementPointer element, void* extraData);
static void bumpTimestamp(EntityItemProperties& properties);
void notifyNewlyCreatedEntity(const EntityItem& newEntity, const SharedNodePointer& senderNode);
@ -332,6 +336,7 @@ protected:
quint64 _totalUpdateTime = 0;
quint64 _totalCreateTime = 0;
quint64 _totalLoggingTime = 0;
quint64 _totalFilterTime = 0;
// these performance statistics are only used in the client
void resetClientEditStats();
@ -351,6 +356,13 @@ protected:
float _maxTmpEntityLifetime { DEFAULT_MAX_TMP_ENTITY_LIFETIME };
bool filterProperties(EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged);
bool _hasEntityEditFilter{ false };
QScriptEngine* _entityEditFilterEngine{};
QScriptValue _entityEditFilterFunction{};
QScriptValue _nullObjectForFilter{};
std::function<bool()> _entityEditFilterHadUncaughtExceptions;
QStringList _entityScriptSourceWhitelist;
};

View file

@ -356,6 +356,7 @@ public:
virtual quint64 getAverageUpdateTime() const { return 0; }
virtual quint64 getAverageCreateTime() const { return 0; }
virtual quint64 getAverageLoggingTime() const { return 0; }
virtual quint64 getAverageFilterTime() const { return 0; }
signals:
void importSize(float x, float y, float z);

View file

@ -0,0 +1,57 @@
function filter(p) {
/* block comments are ok, but not double-slash end-of-line-comments */
/* Simple example: if someone specifies name, add an 'x' to it. Note that print is ok to use. */
if (p.name) {p.name += 'x'; print('fixme name', p. name);}
/* This example clamps y. A better filter would probably zero y component of velocity and acceleration. */
if (p.position) {p.position.y = Math.min(1, p.position.y); print('fixme p.y', p.position.y);}
/* Can also reject altogether */
if (p.userData) { return false; }
/* Reject if modifications made to Model properties */
if (p.modelURL || p.compoundShapeURL || p.shape || p.shapeType || p.url || p.fps || p.currentFrame || p.running || p.loop || p.firstFrame || p.lastFrame || p.hold || p.textures || p.xTextureURL || p.yTextureURL || p.zTextureURL) { return false; }
/* Clamp velocity to maxVelocity units/second. Zeroing each component of acceleration keeps us from slamming.*/
var maxVelocity = 5;
if (p.velocity) {
if (Math.abs(p.velocity.x) > maxVelocity) {
p.velocity.x = Math.sign(p.velocity.x) * maxVelocity;
p.acceleration.x = 0;
}
if (Math.abs(p.velocity.y) > maxVelocity) {
p.velocity.y = Math.sign(p.velocity.y) * maxVelocity;
p.acceleration.y = 0;
}
if (Math.abs(p.velocity.z) > maxVelocity) {
p.velocity.z = Math.sign(p.velocity.z) * maxVelocity;
p.acceleration.z = 0;
}
}
/* Define an axis-aligned zone in which entities are not allowed to enter. */
/* This example zone corresponds to an area to the right of the spawnpoint
in your Sandbox. It's an area near the big rock to the right. If an entity
enters the zone, it'll move behind the rock.*/
var boxMin = {x: 25.5, y: -0.48, z: -9.9};
var boxMax = {x: 31.1, y: 4, z: -3.79};
var zero = {x: 0.0, y: 0.0, z: 0.0};
if (p.position) {
var x = p.position.x;
var y = p.position.y;
var z = p.position.z;
if ((x > boxMin.x && x < boxMax.x) &&
(y > boxMin.y && y < boxMax.y) &&
(z > boxMin.z && z < boxMax.z)) {
/* Move it to the origin of the zone */
p.position = boxMin;
p.velocity = zero;
p.acceleration = zero;
}
}
/* If we make it this far, return the (possibly modified) properties. */
return p;
}