Merge code supporting 3D content in IIIF manifests into manifesto#159
Merge code supporting 3D content in IIIF manifests into manifesto#159vincentmarchetti wants to merge 332 commits intomainfrom
Conversation
is*Transform attributes
as color-string npm package
with unit testing and documentation
the raw representation of intensity is supported.
a default ambient lighting
…atter is past its sell-by date.
Merge base manifesto upstream changes into manifesto-3d
had to manually include docs/.nojekyll and docs/index.html from manifest (2d) code
from the mainfesto/main side of the merge
the 'test' script still runs all tests including the 3d tests
there is no analogous file in that repository and not sure of this file is relevant anymore
in manifesto branch
At the Aug 26 2025 IIIF-Commons call it was agreed to retain the existing manifesto workflows.
IIIF-Commons/manifesto version of the release workflow
were appropriate to manifesto restored the manifesto scripts, retained the additional dependencies necessary to build the 3d code.
by the npm run prettify script Changes were generally replacing the "let" keyword with "const" keyword in those places where code analyis said that would be ok
a variable: from let size:string to const size:string
declaration of the size variable down to the point at which it can be declared and initialized in same statement
npm now gives "severe" security warnings about. All defined scripts still run
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
with strategy ort and option theirs
manifests which referred to a resource "PhysicalBody", which is not part of the prezi-4 draft and which, by design, causes an Error to be thrown from the current commit of AnnotationBodyParser.ts
src/Manifest.ts
Outdated
| if (this._annotationIdMap == null) { | ||
| this._annotationIdMap = {}; | ||
| for (var seq of this.getSequences()) | ||
| for (var scene of seq.getScenes()) |
There was a problem hiding this comment.
Since this only works on scenes, should the name reflect that. Also should this be a getter function for consistency?
There was a problem hiding this comment.
I think the more appropriate fix would be to extend this function to gather Annotation from Canvas resources as well, and any other Containers defined in the updates IIIF API spec (4)
I lean toward retaining this as a class member with a get keyword
There was a problem hiding this comment.
Have made the change, so the annotationIdMap includes Annotations in the Content of any Canvas of the manifest at commit fc22c16
src/SpecificResource.ts
Outdated
| get Source(): object | AnnotationBody { | ||
| return this.getSource(); | ||
| } |
There was a problem hiding this comment.
a getter and a method feels confusing, given the structure of the other APIs
There was a problem hiding this comment.
I agree, I am responsible for those and others working on the 3D extensions have expressed that concern, so those doubled-up get X() and getX() cases will be simplified to just one method
There was a problem hiding this comment.
At 83ac881 have removed the cases where a getter just duplicated an existing method returning the same date.
I propose this conversation can be marked as resolved
| get Position(): SpecificResource | null { | ||
| return this.getPosition(); | ||
| } |
There was a problem hiding this comment.
Another instance of the getter/method combination.
There was a problem hiding this comment.
I agree, as before
| constructor(jsonld?: any, options?: IManifestoOptions) { | ||
| super(jsonld, options); | ||
| } |
There was a problem hiding this comment.
Textual bodies may have multiple languages, is that something that should be considered at this point? Should the getValue() return a multilingual string like label and summary?
There was a problem hiding this comment.
Will check for consistency with other classes delivering multilingual options
There was a problem hiding this comment.
According to the web annotation model document, at #embedded-textual-body the TextualBody class can have a language property, but each resource only provided a value for a single language. I think this means manifest authors would use a Choice resource to provide several TextualBody resources to offer multilingual presentation
There was a problem hiding this comment.
Note we have a recipe for multilingual annotations: https://iiif.io/api/cookbook/recipe/0346-multilingual-annotation-body/
get X() ,property based on the get keyword, just duplicated a getX() function that was already defined
the getValue() function call, for alignment with other classes having a Value property
annotation indexed in the manifest annotationIdMap property.
6 participants
This pull request will merge in the code developed to support the 3D extensions developed for Presentation 4 specification.
Discussed at the August 26 2025 call of the IIIF-Commons group, call minutes at Running Notes
Some notes on the state of this PR
-- All of the new code related to 3D content are included, as well as all of the tests.
-- As we discussed Aug 26, none of the additional workflow/actions developed in the 3D repository are included, in particular in this PR the documentation will continue to be generated manually.
-- The dependency on the module prettier-check has been removed, in response to "severe" security warnings now issued by npm upon install.
-- neither the docs nor the package-lock.json changes are included in the commit of this PR
-- the package naming, website, and GitHub repo are all set to the IIIF-Commons/manifesto values.
-- the version number is unchanged, is at 4.2.22, we agreed that if/when merge is is performed the version would be bumped to the next minor release